Skip to content

Conversation

@celikfatih
Copy link
Contributor

@celikfatih celikfatih commented Nov 17, 2025

This PR implements the enhancement proposed in #953, allowing CORS preflight (OPTIONS) requests to bypass authentication across supported authentication filters.

Browsers perform CORS preflight requests before sending actual cross-origin requests, and these preflight requests must not be forced through authentication in order for the CORS handshake to complete successfully.

This change updates the access-control logic to detect preflight requests via CorsUtils.isPreFlightRequest(...) and immediately allow them when allowPreflightRequests is enabled.
This behavior applies generically and is not limited to Basic authentication.

Key Changes

Added a preflight request check in isAccessAllowed(...) within the relevant filter.

Ensured that OPTIONS requests with valid CORS headers bypass authentication.

Updated Javadoc explaining the new behavior.

Added unit tests for CorsUtils.isPreFlightRequest(...).

Fixes #953

Following this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GitHub issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a GitHub issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [#XXX] - Fixes bug in SessionManager,
    where you replace #XXX with the appropriate GitHub issue. Best practice
    is to use the GitHub issue title in the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • add fixes #XXX if merging the PR should close a related issue.
  • Run mvn verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If you have a group of commits related to the same change, please squash your commits into one and force push your branch using git rebase -i.
  • Committers: Make sure a milestone is set on the PR

Trivial changes like typos do not require a GitHub issue (javadoc, comments...).
In this case, just format the pull request title like [DOC] - Add javadoc in SessionManager.

If this is your first contribution, you have to read the Contribution Guidelines

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@github-actions github-actions bot added github_actions Pull requests that update GitHub Actions code java Pull requests that update Java code tests labels Nov 17, 2025
Copy link
Contributor

@lprimak lprimak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably != null check is not sufficient, as it could be blank

@lprimak
Copy link
Contributor

lprimak commented Nov 17, 2025

Thank you for your contribution! We really appreciate it.

Couple of issues:
Tests do not include copyright header, this is why build fails.
Also looks like your branch needs to be updated with main branch.

@lprimak lprimak added pending-cla valid Disable automation for valid issues and removed github_actions Pull requests that update GitHub Actions code tests labels Nov 17, 2025
@lprimak
Copy link
Contributor

lprimak commented Nov 17, 2025

Also, since you removed the issue template, there is no way to know of your copyright attribution to Apache.
Can you please restore the template for this PR and attribute appropriate copyrights / CLA to Apache.

Thank you

@github-actions github-actions bot added github_actions Pull requests that update GitHub Actions code tests labels Nov 17, 2025
Copy link
Contributor

@lprimak lprimak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few minor comments

@lprimak lprimak removed the github_actions Pull requests that update GitHub Actions code label Nov 17, 2025
@lprimak
Copy link
Contributor

lprimak commented Nov 17, 2025

I am not sure why this PR is showing differences from main when those changes are already in main... I have never seen that before, but it's not correct.

Are you by any chance cherry-picking latest commits into main into your branch?
Don't do that... just git merge main

@celikfatih
Copy link
Contributor Author

I am not sure why this PR is showing differences from main when those changes are already in main... I have never seen that before, but it's not correct.

Are you by any chance cherry-picking latest commits into main into your branch? Don't do that... just git merge main

No, I'm not doing that; I'm just following the instructions in the CONTRIBUTING.MD file.

git rebase main

@github-actions github-actions bot added github_actions Pull requests that update GitHub Actions code and removed github_actions Pull requests that update GitHub Actions code labels Nov 17, 2025
@celikfatih
Copy link
Contributor Author

@lprimak I used git merge main, I think everything is ok now.

@lprimak lprimak added this to the 2.0.7 milestone Nov 18, 2025
Copy link
Contributor

@lprimak lprimak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there...
Need to correct the checkstyle errors.
Also, do you know how to squash the commits?
There should be only one commit in the PR.

[INFO] --- checkstyle:3.6.0:checkstyle (default) @ shiro-web ---
 [INFO] Rendering content with org.apache.maven.skins:maven-fluido-skin:jar:2.1.0 skin
 [INFO] Starting audit...
 [ERROR] web/src/main/java/org/apache/shiro/web/filter/authc/HttpAuthenticationFilter.java:82:21: Variable 'allowPreFlightRequests' explicitly initialized to 'false' (default value for its type). [ExplicitInitialization]
 [ERROR] web/src/main/java/org/apache/shiro/web/util/CorsUtils.java:65:53: '&&' should be on a new line. [OperatorWrap]
 [ERROR] web/src/main/java/org/apache/shiro/web/util/CorsUtils.java:66:64: '&&' should be on a new line. [OperatorWrap]
 Audit done.

@lprimak
Copy link
Contributor

lprimak commented Nov 18, 2025

@fpapon @bmarwell
Should this be false by default? I believe there is no harm to enable this by default... What do you think?

@celikfatih
Copy link
Contributor Author

celikfatih commented Nov 18, 2025

Almost there...
Need to correct the checkstyle errors.
Also, do you know how to squash the commits?
There should be only one commit in the PR.

Yes I can handle this, I will also fix your checkstyle problems.

Note: I merged all commits into the one.

@celikfatih celikfatih force-pushed the feature-preflight-support branch 2 times, most recently from 9dfaef8 to 20ad921 Compare November 18, 2025 11:50
@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Nov 18, 2025
@celikfatih celikfatih force-pushed the feature-preflight-support branch from 20ad921 to 852cc7f Compare November 18, 2025 11:54
@github-actions github-actions bot removed the github_actions Pull requests that update GitHub Actions code label Nov 18, 2025
@lprimak
Copy link
Contributor

lprimak commented Nov 18, 2025

Approved. Tests were failing due to CloudFlare outage

@lprimak lprimak added CLA and removed pending-cla labels Nov 18, 2025
@fpapon fpapon requested a review from bmarwell November 18, 2025 19:35
@fpapon
Copy link
Member

fpapon commented Nov 18, 2025

@fpapon @bmarwell Should this be false by default? I believe there is no harm to enable this by default... What do you think?

Having it enable by default can make sense but it could change the default behavior for users after upgrade the version.

@lprimak
Copy link
Contributor

lprimak commented Nov 18, 2025

I'll take that as a no...sounds good

Having it enable by default can make sense but it could change the default behavior for users after upgrade the version.

@fpapon fpapon self-requested a review November 18, 2025 20:01
Copy link
Member

@fpapon fpapon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as we don't enable it by default for now. May be in next major version as a breaking change.

@fpapon
Copy link
Member

fpapon commented Nov 18, 2025

and @celikfatih thank you for your contribution!

@lprimak
Copy link
Contributor

lprimak commented Nov 18, 2025

See #2376 for 3.x to change the default behavior

Copy link
Contributor

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use mockito + assertj even in the one other test case

@celikfatih celikfatih force-pushed the feature-preflight-support branch from 67f563f to 416c776 Compare November 21, 2025 11:22
@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Nov 21, 2025
@celikfatih celikfatih force-pushed the feature-preflight-support branch from 416c776 to 3af59b5 Compare November 21, 2025 11:31
@github-actions github-actions bot removed the github_actions Pull requests that update GitHub Actions code label Nov 21, 2025
@celikfatih
Copy link
Contributor Author

celikfatih commented Nov 21, 2025

I replaced the new tests with assertj and mockito @bmarwell @lprimak @fpapon. Sorry it took so long, I'm new to this project and open source contributions and I'm still learning 😄

@lprimak
Copy link
Contributor

lprimak commented Nov 21, 2025

I'm new to this project and open source contributions and I'm still learnin

You are doing great. Thank you! Love the persistence!

Copy link
Contributor

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the CorsUtils should be a public final utility class instead, but I do not want to decide that alone if you are okay with it.

However, I want to menteion: I love all the javadoc you added as well as the corrections you did! Thank you so much! :D

@lprimak lprimak merged commit 8cbc664 into apache:main Nov 28, 2025
30 checks passed
this.authcScheme = authcScheme;
}

public void setAllowPreFlightRequests(boolean allowPreFlightRequests) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (might) be missing isAllowPreflightRequests() getter - but I am not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is missing. Should we add it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA java Pull requests that update Java code tests valid Disable automation for valid issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add preflight support to HttpAuthenticationFilter

4 participants