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

Fix-cache-control #189

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Similadayo
Copy link

This pull request fixes a problem with the GetHeaders method in the OAuth and JWT Bearer security schemes. It wasn't handling cases where attack and valid values were nil, which could cause errors. I changed the code to fix this, making sure headers are generated more reliably for requests.

Changes:

  1. Fixed errors with type checks in GetHeaders.

  2. Improved how attack and valid values are handled in headers.

I look forward to your feedback on these updates!

Add cache-control header to the OAuthSecurityScheme GetHeaders() function based on the presence of a valid value. If a valid value is present, set the cache-control header to "private, max-age=0". Otherwise, set it to "public, max-age=3600".
@emmanuelgautier
Copy link
Member

Thanks for contributing @Similadayo.

I'm not sure which type check error this PR is addressing. Could you maybe share an example of the error you encountered during the scan?

Regarding the changes in your PR, I see you've added Cache-Control request headers. It's true that APIs should not allow public cacheable responses if authentication is required. Testing for this behavior is actually planned for the next version (see issue #187). If you're interested in working on this feature as well. I will be happy to welcome your contributions!

@Similadayo
Copy link
Author

Thanks for getting back to me @emmanuelgautier

My PR tackles the same issue about Cache-Control headers for authenticated APIs. It makes sure that public cacheable responses aren’t allowed when authentication is used, which helps protect confidential data. I’d love to contribute more to this issue and help with testing in the next version. Let me know how I can help.

@emmanuelgautier
Copy link
Member

The goal of the scan is not necessarily to ensure that requests made during the scan aren't cached, but rather to check if there are any shared cache directives (like public Cache-Control) in the response, which could expose sensitive data.

I think, you should add a new scan for this specific case. You can refer to one of the existing scans in this repository: https://github.com/cerberauth/vulnapi/tree/main/scan.

I'd be happy to assist. One approach for the scan would involve making a request with an authentication method in place and then inspecting the response headers. If the response is successful (e.g., 2xx status code), and it includes public cache control directives, the test would fail as this could expose protected data. Do not hesitate to suggest another approach if you think there’s a better way to perform this scan.

@Similadayo
Copy link
Author

Thanks for the clarification.

I will go through the scan directory to understand better and won't hesistate to reach out incase I need understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants