Skip to content

Convert pagination ISE into a Bad Request response #2065

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Jan 10, 2020

Conversation

jtgeibel
Copy link
Member

@jtgeibel jtgeibel commented Dec 25, 2019

Errors in parsing the pagination query string parameters can be safely returned to the client and do not need to propagate up to the logging middleware.

The following have been observed in production logs:

  • error="cannot parse integer from empty string" for /api/v1/crates?page=&per_page=50&sort=downloads (empty page=)
  • error="invalid digit found in string" for /api/v1/crates?page=1&per_page=100%22%EF%BC%8Cexception" (very invalid per_page)

Additionally, 2 existing pagination errors are converted to use bad_request() in place of cargo_err(). It is not necessary to send a status=200 response to support old versions of cargo for pagination errors.

Fixes: #1928

r? @smarnach

@carols10cents
Copy link
Member

This will fix #1928 :)

@jtgeibel
Copy link
Member Author

jtgeibel commented Jan 3, 2020

Thanks! I've updated the description to reference that issue, and cherry-picked the tests provided there.

@bors
Copy link
Contributor

bors commented Jan 6, 2020

☔ The latest upstream changes (presumably #2032) made this pull request unmergeable. Please resolve the merge conflicts.

jtgeibel and others added 3 commits January 5, 2020 22:12
Errors in parsing the pagination query string parameters can be returned
to the client and do not need to propagate up to the logging middleware.

The following have been observed in production logs:

* error="cannot parse integer from empty string" for
  `/api/v1/crates?page=&per_page=50&sort=downloads`
* error="invalid digit found in string" for
  `/api/v1/crates?page=1&per_page=100%22%EF%BC%8Cexception"`
Reviewing the source for cargo, it does not appear that there is any way
to specify a `page` number and the `per_page` value is capped at 100 by
cargo since at least 1.29.  Showing a sub-optimal error message to very
old versions of cargo seems okay.
@jtgeibel jtgeibel force-pushed the prod/make-2-errors-user-facing branch from 1f53043 to 741093f Compare January 6, 2020 03:15
@smarnach
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 10, 2020

📌 Commit 741093f has been approved by smarnach

@bors
Copy link
Contributor

bors commented Jan 10, 2020

⌛ Testing commit 741093f with merge c7a37b3...

bors added a commit that referenced this pull request Jan 10, 2020
…nach

Convert pagination ISE into a Bad Request response

Errors in parsing the pagination query string parameters can be safely returned to the client and do not need to propagate up to the logging middleware.

The following have been observed in production logs:

* error="cannot parse integer from empty string" for `/api/v1/crates?page=&per_page=50&sort=downloads` (empty `page=`)
* error="invalid digit found in string" for `/api/v1/crates?page=1&per_page=100%22%EF%BC%8Cexception"` (very invalid `per_page`)

Additionally, 2 existing pagination errors are converted to use `bad_request()` in place of `cargo_err()`.  It is not necessary to send a status=200 response to support old versions of cargo for pagination errors.

Fixes:  #1928

r? @smarnach
@bors
Copy link
Contributor

bors commented Jan 10, 2020

☀️ Test successful - checks-travis
Approved by: smarnach
Pushing c7a37b3 to master...

@bors bors merged commit 741093f into rust-lang:master Jan 10, 2020
@jtgeibel jtgeibel deleted the prod/make-2-errors-user-facing branch January 24, 2020 02:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only accept integers for page and per_page params; return 400 otherwise
5 participants