CORS is not checked when browsing files. check origin now https://github.com/jupyter-server/jupyter_server/issues/1459

[gogasca]

Add check_origin check during check_xsrf_cookie for files.
Details: #1459

Summary

This PR addresses a CORS bypass vulnerability in the /files endpoint. The issue allows attackers to bypass
configured CORS restrictions (allow_origin_pat) by exploiting the XSRF token validation flow.

The Vulnerability

The current check_xsrf_cookie() method in base/handlers.py:537 only calls super().check_xsrf_cookie() (Tornado's
XSRF validation) without first checking if the request origin is allowed. This allows attackers to:

1. Set an XSRF cookie in a shared domain
2. Make cross-origin requests with matching XSRF tokens
3. Access /files endpoints despite CORS restrictions

Risk Assessment:

• Impact: Unauthorized access to server files
• Exploitability: Requires shared domain configuration but relatively straightforward to exploit
• Scope: Affects all file access via /files endpoint
• Mitigation: Simple fix with clear security benefit

[vidartf]

@gogasca Thanks for the issue and the PR. I've been reading it, and agree that the allow_origin_pat should be checked in this case as well. That said, in the exception handler there is already some code to call check_referer for GET and HEAD, and the docstring of that method already specifically mentions the /files endpoint, so I think it should probably be the method used here (at least for GET/HEAD). Also, I want to be careful to see if there are any configuration scenarios where the extra check would fail some existing behavior that we still want to allow, so I've been discussing this in the dev call (cc @Zsailer ), so the review might take a bit longer.

Happy for any input you have on the above, and thanks again!

[gogasca]

Thanks @vidartf I will test using check_referer method and get back to you. I have been unable to join the call in the past few weeks due to some personal commitments, but will report back with more details. Thanks

[gogasca]

Change self.check_origin() to check_referer()

[gogasca]

After changing: Change self.check_origin() to check_referer() many tests cases are broken, reverting back to self.check_origin()

[gogasca]

@vidartf can you PTAL again?

[gogasca]

@vidartf @Zsailer when you have some time, could you PTAL at this PR. Thanks!

[gogasca]

@vidartf @Zsailer when you have some time, could you PTAL at this PR. Thanks!

[Zsailer]

@gogasca apologies that this has taken so long to review. I updated the top level comment to give more information here.

@vidartf You raise a valid concern about consistency with existing behavior. Here's my analysis:

Key Differences Between Methods

check_origin():

• Allows requests with missing Origin header (common for same-origin requests, scripts)
• General CSRF protection for API requests and WebSockets
• More permissive

check_referer():

• Blocks requests with missing Referer header
• Specifically designed for /files/ endpoint (per docstring: "Used on GET for api endpoints and /files/ to block
  cross-site inclusion (XSSI)")
• More restrictive, better XSSI protection

The Trade-off

Your suggestion to use check_referer() for GET/HEAD requests is technically correct for consistency, but
introduces compatibility risks:

• ✅ Better security: Purpose-built for /files/ endpoint XSSI protection
• ✅ Consistency: Matches existing fallback behavior
• ⚠️ Compatibility risk: May break direct file access, bookmarked files, or scenarios where browsers strip
  Referer headers

My Recommendation

• Option 1: Hybrid Approach (if we want architectural consistency)

 try:
      if self.request.method in {"GET", "HEAD"}:
          if not self.check_referer():
              raise web.HTTPError(404)
      else:
          if not self.check_origin():
              raise web.HTTPError(404)
      return super().check_xsrf_cookie()
  except web.HTTPError as e:
      # Existing fallback logic remains unchanged

• Option 2: The current approach is a safe, minimal fix that:
  • ✅ Fixes the immediate CORS bypass vulnerability
  • ✅ Preserves existing fallback behavior
  • ✅ Lower risk of breaking legitimate workflows

Conclusion

I think the current PR is acceptable as-is for the security fix. The architectural improvement (using
check_referer for GET/HEAD) could be addressed in a follow-up PR with more extensive testing to ensure no
regressions.

[Zsailer]

Thank you @gogasca for your patience here!