Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Enhancement: only expose the path in ValidationExceptions #3061

Closed
floxay opened this issue Feb 1, 2024 · 6 comments · Fixed by #3064
Closed

Enhancement: only expose the path in ValidationExceptions #3061

floxay opened this issue Feb 1, 2024 · 6 comments · Fixed by #3064
Assignees
Labels
Enhancement This is a new feature or request

Comments

@floxay
Copy link
Contributor

floxay commented Feb 1, 2024

Summary

Currently ValidationException exposes the full URL in the error response, leaking internal IP(s) or other similar infra related information.

Relevant code:

return ValidationException(detail=f"Validation failed for {method} {connection.url}", extra=client_errors)
f"Missing required {param.param_type.value} parameter {param.field_alias!r} for url {connection.url}"
{connection.url} -> {connection.url.path}

Basic Example

{
    "status_code": 400,
    "detail": "Missing required query parameter 'q' for url http://127.0.0.1:8000/"
}

and

{
    "status_code": 400,
    "detail": "Validation failed for GET http://127.0.0.1:8000/?q=s",
    "extra": ...
}

would become

{
    "status_code": 400,
    "detail": "Missing required query parameter 'q' for path /"
}
{
    "status_code": 400,
    "detail": "Validation failed for GET /",
    "extra": ...
}

Drawbacks and Impact

No response

Unresolved questions

No response


Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@floxay floxay added the Enhancement This is a new feature or request label Feb 1, 2024
@provinzkraut
Copy link
Member

provinzkraut commented Feb 2, 2024

Sounds good to me. It seems like you already figured out what changes need to be made here. Do you want to open a PR for this?

@floxay
Copy link
Contributor Author

floxay commented Feb 2, 2024

I have just realized that by simply doing url.path the query parameters will be gone as well.

So, for example;
Validation failed for GET http://127.0.0.1:8000/?q=s
would become
Validation failed for GET /
instead of the originally mentioned
Validation failed for GET /?q=s

It makes sense for the 'feature request' as it really is just the path that's there but my example was incorrect. (I have now edited it in the original post)

@floxay
Copy link
Contributor Author

floxay commented Feb 14, 2024

Shouldn't have this auto-closed when #3064 got merged? Is it some Github feature/behavior I am unaware of or is it still open on purpose?

@provinzkraut
Copy link
Member

The PR was merged into develop. GitHub does not consider this merged as it's not the default branch. We do have a workflow that's supposed to close these issues, but it currently has an issue when the PR originates from a fork (see the failing workflow here).

@provinzkraut
Copy link
Member

Closed in #3064.

Copy link

A fix for this issue has been released in v2.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request
Projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

2 participants