Skip to content

Harden XSS - #5832

Open
multimeric wants to merge 3 commits into
OSC:masterfrom
multimeric:harden-xss
Open

multimeric wants to merge 3 commits into
OSC:masterfrom
multimeric:harden-xss

Conversation

@multimeric

@multimeric multimeric commented Sep 23, 2026

Copy link
Copy Markdown

Please review our Contributing Guide before submitting a pull request.

What does this PR do, and what is the related issue, if applicable?

Adds two lines of defence for XSS from untrusted apps:

  • Adds a Content-Security-Policy (connect-src and form-action) to interactive apps that restricts their ability to make requests outside of that individual app URL
  • Adds a proxy rewrite rule for the PUN (dashboard etc) that only allows requests that are Sec-Fetch-Dest: document (allowing you to browse there directly), or that have a Referer header which is the PUN itself.

Fixes #5824

Testing

  • Tests included
  • If a maintainers' assistance is needed for tests, add label test help
  • No test is needed because _____ (documentation fix, dependency update, etc.)

Questions

  • Should this behaviour be configurable, ie should we allow users to switch this off in case it breaks some edge case?

Documentation

Hardens OnDemand against potentially malicious interactive apps, or malicious HTML served by interactive apps

Code Authorship & Understanding

  • I can explain what this code does and answer questions about it
  • This PR was generated or assisted by AI tools (Copilot, Claude, agents). If so, I have reviewed and tested the code
    and I take responsibility for its correctness.

Checklist

  • Follows project code style and conventions
  • This is a large pull request and was discussed first in an issue or with the maintainers.

Anything else?

I only made two small edits. The rest of the changes are updating the fixture files which causes a lot of diff noise.


# Helper that builds a Header directive to set the Content-Security-Policy for a given uri
def node_csp_header(uri)
origin = "#{@protocol}#{@servername || @proxy_server}#{uri}/%{MATCH_HOST}e/%{MATCH_PORT}e/"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think proxy server should take precedence here - I wonder if there's a helper somewhere to determine the actual client facing host.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay, I'll have to think about this one, the architecture of OOD complex. Is there a diagram somewhere? Do we necessarily know which server is serving up a given interactive app? If not, I could just allow both?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the proxy server is enabled it's in front of apache. Think about an HAProxy in front of 2 OOD instances like so

                           site1.edu
                         /
site.edu (proxy_server) <
                         \
                           site2.edu

Which is to say - that's what the browser/user would see and interact with - site.edu.

Seems like you can use @proxy_server as it'll default to the servername if it's not set.

@proxy_server = opts.fetch(:proxy_server, servername)

Comment on lines +398 to +403
# Reject requests to the PUN that didn't come from within pun_uri (e.g. an
# interactive app proxied at node_uri/rnode_uri), unless it's a real page load
RewriteEngine On
RewriteCond %{HTTP:Sec-Fetch-Dest} !=document
RewriteCond %{HTTP_REFERER} !^https?://[^/]+<%= @pun_uri %>(/|$)
RewriteRule ^ - [F]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@treydock I think I'd like your take on this as well.

At a glance it seems OK - though I'm wondering about edge cases, though I can't think of any we need to account for. HTTP_REFERER from another site (like a link from a documentation page) will be a document so that's fine.

Do we also need Sec-Fetch-Site or is it redundant?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Looks like Sec-Fetch-Site can at most distinguish cross-origin which isn't helpful here because both servers have the same domain so the same origin. But I still want to prevent cross communication

@GlazerMann

Copy link
Copy Markdown
Contributor

I think connect-src will block WebSockets. A https://... CSP source does not match wss://... under CSP's scheme-matching rules. Open OnDemand currently supports applications that depend on WebSocket/Websockify traffic.

If I remember correctly, a same-origin interactive app can change its displayed/current URL to /pun/... with history.pushState() without causing navigation. That URL is then used as the Referer for subsequent requests. That reduces the usefulness of Referer as a check.

@multimeric

Copy link
Copy Markdown
Author

I think connect-src will block WebSockets. A https://... CSP source does not match wss://... under CSP's scheme-matching rules. Open OnDemand currently supports applications that depend on WebSocket/Websockify traffic.

Okay, I can specifically build the ws:// URL and add it to the connect-src list. Does that work?

If I remember correctly, a same-origin interactive app can change its displayed/current URL to /pun/... with history.pushState() without causing navigation. That URL is then used as the Referer for subsequent requests. That reduces the usefulness of Referer as a check.

Hmm why can it do this? Is there a better way to stop interactive apps from sending traffic to the dashboard then? I suppose I could also add /pun/app/name (or whatever that URL is) to the allow list. The intention is just to block cross-app communication like an interactive app hitting the dashboard APIs

@multimeric

multimeric commented Sep 24, 2026

Copy link
Copy Markdown
Author

If I remember correctly, a same-origin interactive app can change its displayed/current URL to /pun/... with history.pushState() without causing navigation. That URL is then used as the Referer for subsequent requests. That reduces the usefulness of Referer as a check.

Ohh I think I understand now. You're saying that a malicious script could spoof its Referer by using pushState(). That's a good point. But it's covered by my second line of defence which is the connect-src, because that just checks the URL being requested, not the Referer.

I'm open to cutting the Referer block if we don't think it's adding any meaningful safety. It would at least make this design simpler.

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

Projects

Status: Awaiting Review

Development

Successfully merging this pull request may close these issues.

Harden against XSS in interactive apps

4 participants