[SPARK-59353][UI] Avoid pre-fetch and crawlers triggering actions in UI - #58640
[SPARK-59353][UI] Avoid pre-fetch and crawlers triggering actions in UI#58640holdenk wants to merge 2 commits into
Conversation
The job/stage kill and application hold/resume endpoints, and the Master UI's application/driver kill and application hold/resume endpoints, now require a random per-UI token that the pages embed in the links and forms they render. A cross-site page can neither read the token (same-origin policy) nor guess it, so it cannot drive these endpoints. The same handlers also refuse requests that identify themselves as link prefetches (Purpose, Sec-Purpose, X-Moz) and refuse HEAD outright, since HttpServlet.doHead delegates to doGet and HEAD is required to be safe. The token lives on WebUI, so the application UI and the Master UI each get one without repeating the generator. spark.ui.killViaGetEnabled now follows the cluster manager when it is not set: GET is accepted when spark.master is yarn, because the YARN ResourceManager/AM proxy does not forward POST (SPARK-6846), and refused otherwise. The token rides in the request parameters either way, which such proxies do forward. Setting the config explicitly still wins in both directions. /workers/kill is deliberately left alone. It renders no form, it is documented as an endpoint operators call directly, and a caller with no token could not send one, so requiring one would break it. It stays gated by POST, modify ACLs and spark.master.ui.decommission.allow.mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca>
…eeds The block asserted a 200 on a kill inside eventually. That request kills the job, which removes the kill link, so if anything later in the same iteration failed, every retry then failed while scraping the token and the report blamed a missing token rather than the real problem. Retry only until the link appears; run the rest once, with the requests that actually kill last. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca>
|
Thanks for working on this. The approach looks solid, and the test coverage is great — unit tests plus Selenium coverage for the GET/POST/HEAD/prefetch/missing-token/wrong-token cases and both render modes. A couple of points:
|
| val purpose = Option(request.getHeader("Sec-Purpose")) | ||
| .orElse(Option(request.getHeader("Purpose"))) | ||
| purpose.exists(_.toLowerCase(Locale.ROOT).contains("prefetch")) || | ||
| request.getHeader("X-Moz") != null |
There was a problem hiding this comment.
Minor consistency nit: request.getHeader("X-Moz") != null treats any X-Moz value as a prefetch, whereas the Sec-Purpose/Purpose checks above require the value to contain "prefetch". Matching "prefetch" here too would keep the three branches consistent — e.g. fold X-Moz into the same .exists(_.toLowerCase(Locale.ROOT).contains("prefetch")) test. In practice X-Moz is only ever prefetch, so this is cosmetic.
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
Reviewed the CSRF-token + prefetch/HEAD protection on the state-changing UI endpoints (design and correctness). The per-UI random token threads from WebUI.csrfToken through the tabs into every guarded link/form -- job & stage kill, hold/resume, and the master app/driver forms -- and createRedirectHandler rejects prefetch (403), missing/invalid token (403), and HEAD (405). /workers/kill is intentionally left unguarded, and the tests exercise the GET/POST/HEAD/prefetch/valid/invalid paths. No blocking issues.
One minor, non-blocking note: with spark.ui.killViaGetEnabled on, the token travels in the GET query string, so it lands in server access logs and browser history (cross-origin Referer leakage is mostly covered by the modern strict-origin-when-cross-origin default); the POST-form path avoids that. The X-Moz (any value) vs. Sec-Purpose/Purpose (must contain "prefetch") asymmetry in isPrefetchRequest was already raised in review.
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
The PR description seems outdated. It only mentions POST vs GET, but this PR also requires a per-UI CSRF token (403 on a missing or invalid token), rejects prefetch (403) and HEAD (405) requests, and changes the Master UI endpoints. Could you update the description, including the test section?
| .booleanConf | ||
| .createWithDefault(true) | ||
|
|
||
| val UI_KILL_VIA_GET_ENABLED = ConfigBuilder("spark.ui.killViaGetEnabled") |
There was a problem hiding this comment.
This new config needs .withBindingPolicy(...). This is the cause of the SparkConfigBindingPolicySuite failure (Config enforcement for bindingPolicy) in the hive - other tests job. Like UI_HOLD_ENABLED above, .withBindingPolicy(ConfigBindingPolicy.NOT_APPLICABLE) should work.
| "reject prefetch requests (Purpose/Sec-Purpose/X-Moz headers) and HEAD requests, so " + | ||
| "forged cross-site requests and incidental link fetches cannot trigger them; " + | ||
| "prefetch rejection relies on the prefetcher identifying itself via those headers. " + | ||
| "Introduced in 4.3.0; also available in 3.5.10, 4.0.5, 4.1.4 and 4.2.1; and in all " + |
There was a problem hiding this comment.
Could you remove this sentence here and in docs/configuration.md? Spark docs don't list backport versions in the description, and these releases don't exist yet. In addition, this patch cannot be backported as-is: java.util.HexFormat requires Java 17 (branch-3.5 is on Java 8), and the hold/resume endpoints exist only since 4.4.0.
| httpMethods = killHttpMethods, csrfToken = Some(csrfToken))) | ||
| attachHandler(createRedirectHandler( | ||
| "/jobs/hold", "/jobs/", jobsTab.handleHoldRequest, httpMethods = Set("GET", "POST"))) | ||
| "/jobs/hold", "/jobs/", jobsTab.handleHoldRequest, httpMethods = Set("GET", "POST"), |
There was a problem hiding this comment.
/jobs/hold and /jobs/resume still accept GET regardless of spark.ui.killViaGetEnabled, and AllJobsPage renders them as <a href=".../jobs/hold/?csrfToken=..."> links. Since the token is in the link, a crawler or agent that follows the links of the jobs page without prefetch headers can still hold the application outside YARN, which is the scenario this PR aims to prevent. Shall we apply the same GET/POST policy (a POST form in POST-only mode) to hold/resume? In that case, a config name that is not limited to kill may fit better.
| Either way the state-changing endpoints require the random per-UI CSRF token embedded | ||
| in the links and forms the UI renders, and reject prefetch requests (identified by | ||
| the Purpose, Sec-Purpose, or X-Moz headers) and HEAD requests, so forged cross-site | ||
| requests and incidental link fetches cannot trigger them. Scripted clients can read |
There was a problem hiding this comment.
docs/monitoring.md (/applications/[app-id]/holdstatus) still describes /jobs/hold/ and /jobs/resume/ as POST endpoints which only require modify permissions. Could you mention the csrfToken requirement there too?
| val client = new HttpClient() | ||
| client.start() | ||
| try { | ||
| eventually(timeout(5.seconds), interval(50.milliseconds)) { |
There was a problem hiding this comment.
This test still runs the requests that kill the stage inside eventually, which is the pattern fixed by the second commit for the other tests. If an assertion fails after the first successful POST, every retry fails in scrapeCsrfToken because the kill form is gone. Could you retry only the token scraping here too?
| TestUtils.httpResponseCode(url, "POST") should be (200) | ||
| val base = sc.ui.get.webUrl.stripSuffix("/") | ||
| val token = scrapeCsrfToken(sc) | ||
| // holdEnabled is off in this context, so the actions themselves no-op; what |
There was a problem hiding this comment.
nit. spark.ui.holdEnabled is true by default. These requests are no-ops because executorHoldSupported is false in local mode.
| class="btn btn-sm btn-outline-danger kill-link float-end">Kill</a> | ||
| val killMessage = s"Are you sure you want to kill job ${job.jobId} ?" | ||
| if (killViaGetEnabled) { | ||
| // Default: a plain GET link, which also works through proxies that do not forward |
There was a problem hiding this comment.
nit. This comment is stale because GET is no longer the default outside YARN. The same comment exists in StageTable.scala.
4 participants
What changes were proposed in this pull request?
Make more side-effect causing behavior in the Spark Web UI POST v.s. GET when not deployed on YARN.
Why are the changes needed?
Browser pre-fetch and agents making assumptions on POST v.s. GET "verbs." In general GETs should be side-effect free which is why things like crawlers browsers and agents may choose to prefect them which when the GET is not side-effect free causes undesired behavior.
Does this PR introduce any user-facing change?
Yes new config flag, also pressing back on POST pages often behaves differently in browsers.
How was this patch tested?
New UI test
Was this patch authored or co-authored using generative AI tooling?
Claude and Cursor