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

DefaultBearerTokenResolver triggers processing of multipart content #10326

Closed
pneuschwander opened this issue Sep 27, 2021 · 3 comments · Fixed by #10340
Closed

DefaultBearerTokenResolver triggers processing of multipart content #10326

pneuschwander opened this issue Sep 27, 2021 · 3 comments · Fixed by #10340
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: breaks-passivity A change that breaks passivity with the previous release type: bug A general bug
Milestone

Comments

@pneuschwander
Copy link
Contributor

pneuschwander commented Sep 27, 2021

Affected Artifact:

      <groupId>org.springframework.security</groupId>
      <artifactId>spring-security-oauth2-resource-server</artifactId>
      <version>5.4.7</version>

Describe the bug
Given that JWT Authentication is configured for a simple Spring Boot Web Application (http.oauth2ResourceServer(OAuth2ResourceServerConfigurer::jwt);) and I protect my endpoints (http.authorizeRequests(requests -> requests.mvcMatchers("/api/**").authenticated());)

when I upload a large file to a controller (@RequestPart(name = "file") MultipartFile file) without proving a Bearer Token or with invalid Bearer Token (e.g. using Postman)

then the large file is uploaded and processed by the application before I receive the response "401 unauthenticated" (e.g. after 7 Minutes, depending on the file size).

Cause: DefaultBearerTokenResolver calls resolveFromRequestParameters even though isParameterTokenSupportedForRequest will be false (as allowFormEncodedBodyParameter is false by default). The included request.getParameterValues("access_token") causes the consumption of the whole multipart content.

Another Problem: Access Tokens have short life time (e.g. 5 minutes). We found ourselves in the situation that the processing triggered by the resolver (request.getParameterValues("access_token")) takes that long that the token is always considered expired by the subsequent logic of the BearerTokenAuthenticationFilter / JwtAuthenticationProvider. (Therefore classified as bug)

We might want to consider to change the code to resolve from request parameters (and check for multiple bearer tokens in the request) etc. only if isParameterTokenSupportedForRequest evaluates true to overcome the outlined two problems.

To Reproduce
See Description

Expected behavior

  • The response "401 unauthenticated" should be returned quite fast. Not after minutes.
  • The upload of a large file (upload taking longer than JWT life time) should succeed as the according security filter is passed before the file content is processed / uploaded / consumed.

Sample

No sample at hand. Please let me know whether it is necessary to provide one.

@pneuschwander pneuschwander added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Sep 27, 2021
@marcusdacoregio marcusdacoregio added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 27, 2021
@sjohnr
Copy link
Member

sjohnr commented Sep 27, 2021

@regmebaby, thanks for the report. Do you have any interest in submitting a PR to fix the issue in DefaultBearerTokenResolver? I think defaulting the parameterToken to null and only resolving it if isParameterTokenSupportedForRequest(request) returns true would work for your case as well as existing cases.

@sjohnr sjohnr added this to the 5.6.0-RC1 milestone Sep 27, 2021
@pneuschwander
Copy link
Contributor Author

@regmebaby, thanks for the report. Do you have any interest in submitting a PR to fix the issue in DefaultBearerTokenResolver? I think defaulting the parameterToken to null and only resolving it if isParameterTokenSupportedForRequest(request) returns true would work for your case as well as existing cases.

@sjohnr Great idea, I'll give it a try

@pneuschwander
Copy link
Contributor Author

@sjohnr I just created a pull request (gh-10340) 🙂

pneuschwander added a commit to pneuschwander/spring-security that referenced this issue Oct 12, 2021
Before this commit, the DefaultBearerTokenResolver unconditionally
resolved the request parameters to check whether multiple tokens
are present in the request and reject those requests as invalid.

This commit changes this behaviour to resolve the request parameters
only if parameter token is supported for the specific request
according to spec (RFC 6750).

Closes spring-projectsgh-10326
@sjohnr sjohnr added the type: breaks-passivity A change that breaks passivity with the previous release label Oct 13, 2021
sjohnr pushed a commit that referenced this issue Oct 13, 2021
Before this commit, the DefaultBearerTokenResolver unconditionally
resolved the request parameters to check whether multiple tokens
are present in the request and reject those requests as invalid.

This commit changes this behaviour to resolve the request parameters
only if parameter token is supported for the specific request
according to spec (RFC 6750).

Closes gh-10326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: breaks-passivity A change that breaks passivity with the previous release type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants