This is an automated email from the ASF dual-hosted git repository. jamesbognar pushed a commit to branch release-manager in repository https://gitbox.apache.org/repos/asf/juneau.git
commit 49fd9d4448ff64b529d8204fb02db09135a7f6c4 Author: James Bognar <[email protected]> AuthorDate: Tue Aug 18 13:40:20 2026 -0400 TODO-419/434: Wire the loopback write boundary and a CSRF synchronizer token into Release Manager Before this, the only thing gating a mutating request was per-run arming (rm.mode=live plus the typed confirm phrase) -- which answers "did a human mean it", never "did this request come from the page we served". The confirm phrase is derivable from the app's own page, so a hostile page in the operator's browser could POST it to /arm cross-origin with no preflight and no JavaScript (TODO-434), and rm.mode=live being the operational default meant that gap was open exactly during a real release. AppConfiguration mints one SynchronizerToken per boot and registers LoopbackBoundaryFilter at /* with highest precedence. ConsolePage (new) is a CSRF-token-seeded FreemarkerView.of() replacement, adopted across HomeRest / CredentialRest / ReleaseRest / ReleaseRunRest / AdminRest. base.ftlh carries the token in a <meta> tag with no FreeMarker default (a page that forgets ConsolePage fails to render rather than quietly shipping writes that all 403); csrf.js (new) wraps window.fetch so every non-GET call attaches it automatically. Every mutating endpoint on CredentialRest / ReleaseRunRest is now @Mutating-annotated. Both also set disableContentParam so a write payload cannot arrive via ?content= and land in browser history or access logs -- the boundary already refuses that shape from a hostile page; this closes the accidental, same-origin version of it. ReleaseEngine / NexusStagingClient gain a selfCallHeaders() plumbing path so the SAFE-mode Nexus mock's self-calls (same app, same port) carry the boundary's own headers too, since the boundary exempts nobody including this process. New CredentialWriteVectorTest (READY-435) is the forged cross-origin credential-overwrite regression test. ReleaseRestTest and AdminRest are updated for the page(HttpServletRequest) signature ConsolePage requires. Cross-repo build/bisect hazard: this commit depends on juneau master commit -- e1c1394eaa TODO-419: Add a loopback write boundary (CSRF/DNS-rebinding defense) for loopback-bound REST apps Release Manager resolves juneau through versioned Maven coordinates (juneau.version=10.0.0-SNAPSHOT in pom.xml), not a reactor or parent build -- there is no <parent> linking the two poms. LoopbackBoundary, LoopbackBoundaryFilter, SynchronizerToken, Mutating and MethodSafety do not exist before that commit, so this one will not compile against an older ~/.m2 juneau install. Anyone bisecting release-manager across this point needs juneau mvn install-ed at or past it, not merely release-manager checked out. --- .../org/apache/juneau/releng/AppConfiguration.java | 91 ++++++++- .../apache/juneau/releng/engine/ReleaseEngine.java | 33 +++- .../juneau/releng/nexus/NexusStagingClient.java | 22 ++- .../org/apache/juneau/releng/rest/AdminRest.java | 9 +- .../org/apache/juneau/releng/rest/ConsolePage.java | 60 ++++++ .../apache/juneau/releng/rest/CredentialRest.java | 24 ++- .../org/apache/juneau/releng/rest/HomeRest.java | 7 +- .../org/apache/juneau/releng/rest/ReleaseRest.java | 13 +- .../apache/juneau/releng/rest/ReleaseRunRest.java | 37 +++- src/main/resources/application.properties | 14 ++ src/main/resources/static/js/csrf.js | 102 ++++++++++ src/main/resources/templates/base.ftlh | 12 ++ .../releng/rest/CredentialWriteVectorTest.java | 211 +++++++++++++++++++++ .../apache/juneau/releng/rest/ReleaseRestTest.java | 15 +- 14 files changed, 615 insertions(+), 35 deletions(-) diff --git a/src/main/java/org/apache/juneau/releng/AppConfiguration.java b/src/main/java/org/apache/juneau/releng/AppConfiguration.java index 495b99d0e5..865e9b2da7 100644 --- a/src/main/java/org/apache/juneau/releng/AppConfiguration.java +++ b/src/main/java/org/apache/juneau/releng/AppConfiguration.java @@ -57,11 +57,16 @@ import org.apache.juneau.releng.rest.MilestoneRest; import org.apache.juneau.releng.rest.ReleaseRest; import org.apache.juneau.releng.rest.ReleaseRunRest; import org.apache.juneau.releng.util.ProcessRunner; +import org.apache.juneau.rest.server.filter.LoopbackBoundary; +import org.apache.juneau.rest.server.filter.LoopbackBoundaryFilter; +import org.apache.juneau.rest.server.filter.SynchronizerToken; import org.apache.juneau.secret.keychain.KeychainSecretStore; import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.web.servlet.FilterRegistrationBean; import org.springframework.boot.web.servlet.ServletRegistrationBean; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.core.Ordered; import jakarta.servlet.Servlet; @@ -78,6 +83,74 @@ public class AppConfiguration { return new ProcessRunner.Default(); } + // ========================================================================================== + // Loopback write boundary. + // + // Two independent gates protect the mutating endpoints, and they answer different questions: + // + // - This boundary answers "did this request come from the page we served". + // - The engine's per-run arming (see ReleaseEngine.arm / rm.mode) answers "did a human mean it". + // + // Before this boundary existed only the second gate was present, which meant a hostile page in the + // operator's browser could POST the derivable "<version> LIVE" confirm phrase to /arm as a plain + // cross-origin form submission -- no preflight, no CORS, no JavaScript needed -- and then trigger a + // mutating step. Arming establishes intent; it never established that the caller was us. + // + // Two framing points, because getting either wrong invites someone to "strengthen" the wrong gate: + // + // - The confirm phrase is not a secret and never authenticated anything. It is derivable from a page the + // attacker is already reading. Making it longer or hidden would not help -- see ReleaseEngine.arm. It is + // typing friction against accident, which is a real thing to want and not this boundary's job. + // + // - rm.mode defaulting to safe is an operational safety default, not a security control, even though it was + // doing real security work until this boundary landed. Its protection disappears exactly when someone + // starts the app in live mode -- during a real release, when a forged request costs the most. This + // boundary applies in both modes. See application.properties. + // ========================================================================================== + + /** + * The process's CSRF secret: minted here, embedded in every page (see the page beans below), and required + * back on every write. One per boot, so a restart invalidates whatever any still-open tab is holding and + * that tab must reload. + * + * <p>Deliberately <b>not</b> a double-submit cookie: cookies are scoped by host and ignore the port, so any + * page served from any other {@code localhost:*} could plant one this application would read back and + * accept. {@link SynchronizerToken} documents that at length. + */ + @Bean + public SynchronizerToken csrfToken() { + return SynchronizerToken.generate(); + } + + /** + * The boundary itself, pinned to the one authority this application is reached at. + * + * <p>Exactly one spelling is accepted: bound to {@code 127.0.0.1:8790}, the app is <b>not</b> reachable as + * {@code localhost:8790}. That is intentional (see {@link LoopbackBoundary}'s canonical-origin section); a + * 421 with a {@code Host}-naming message is the diagnostic when someone uses the other spelling. + */ + @Bean + public LoopbackBoundary loopbackBoundary(SynchronizerToken token, + @Value("${server.address:127.0.0.1}") String address, @Value("${server.port:8790}") int port) { + return LoopbackBoundary.create().authority(address, port).token(token).build(); + } + + /** + * Registers the boundary as a servlet filter at {@code /*} with the highest precedence. + * + * <p>A filter rather than a Juneau guard or mixin hook because it must be impossible to omit: every REST + * resource, the SSE endpoint, the Nexus mock, the static assets and the 404s all pass through it, so an + * endpoint added later is covered without anyone remembering to cover it. A guard declared per-operation + * would leave each new endpoint unprotected by default, and its absence would be invisible in review. + */ + @Bean + public FilterRegistrationBean<LoopbackBoundaryFilter> loopbackBoundaryFilter(LoopbackBoundary boundary) { + var reg = new FilterRegistrationBean<>(new LoopbackBoundaryFilter(boundary)); + reg.addUrlPatterns("/*"); + reg.setOrder(Ordered.HIGHEST_PRECEDENCE); + return reg; + } + /** * FreeMarker engine configuration picked up by the Juneau view bridge (via BeanStore lookup). * @@ -186,8 +259,7 @@ public class AppConfiguration { /** * The shared console-ui chrome stylesheet + themeable logo/page-background assets, mounted independently of * {@code /rest/*} so the site-absolute {@code /juneau-console/*} URLs every tab's {@code base.ftlh} links - * against resolve the same way regardless of which tab rendered the page. See {@link ConsoleAssetsRest}'s - * class Javadoc for why the servlet itself corrects for the container's servlet-path handling. + * against resolve the same way regardless of which tab rendered the page. */ @Bean public ServletRegistrationBean<Servlet> consoleAssetsRegistration() { @@ -239,7 +311,7 @@ public class AppConfiguration { /** Resolves live secrets from CredentialService's stores per mutating action. */ @Bean public ReleaseEngine.SecretResolver secretResolver(Map<CredentialSpec, SecretStore> stores, AccountStore accounts, - ExecutionMode mode, TargetProfile target) { + ExecutionMode mode, TargetProfile target, LoopbackBoundary boundary) { return new ReleaseEngine.SecretResolver() { private String read(CredentialSpec spec, String account) { return stores.get(spec).find(account).map(String::new).orElse(""); @@ -279,11 +351,14 @@ public class AppConfiguration { // cannot be used directly. Falls back to ~/.m2/settings.xml only when that Keychain entry // hasn't been stored yet. Under SAFE the base is the loopback mock and the credential is a // throwaway placeholder — the real Keychain secret is never read or sent (OQ-C). + // + // The SAFE client also carries the boundary's self-call headers: its target is the mock on this + // application's own port, which sits behind the same filter as everything else and exempts nobody. @Override public NexusStagingClient nexus() { if (mode == ExecutionMode.SAFE) return NexusStagingClient.create(target.nexusBaseUrl(), target.nexusProfileId(), SAFE_PLACEHOLDER, - SAFE_PLACEHOLDER); + SAFE_PLACEHOLDER, boundary.selfCallHeaders()); return nexusClient(target, availid(), ldapPassword(), () -> NexusStagingClient .create(target.nexusBaseUrl(), target.nexusProfileId(), "apache.releases.https")); } @@ -313,13 +388,14 @@ public class AppConfiguration { }) public ReleaseEngine releaseEngine(RunStateStore store, StepRegistry registry, ProcessRunner runner, BranchResolver branches, EmailService email, MilestoneService milestone, - ReleaseEngine.SecretResolver secrets, ExecutionMode mode, TargetProfile target, + ReleaseEngine.SecretResolver secrets, ExecutionMode mode, TargetProfile target, LoopbackBoundary boundary, @Value("${rm.state.dir}") String stateDir, @Value("${rm.staging.dir}") String stagingDir, @Value("${rm.repo.dir}") String repoDir, @Value("${rm.git.committer.email}") String committerEmail, @Value("${server.address:127.0.0.1}") String address, @Value("${server.port:8790}") int port) { var engine = new ReleaseEngine(store, registry, runner, branches, Path.of(stateDir), Path.of(stagingDir), repoDir, committerEmail, email, milestone, secrets, mode, target); engine.setMockNexusBaseUrl("http://"; + address + ":" + port + "/mock/nexus"); + engine.setLoopbackHeaders(boundary.selfCallHeaders()); engine.recoverOnBoot(); // Demote runs left mid-flight by a previous process on restart. return engine; } @@ -327,12 +403,13 @@ public class AppConfiguration { @Bean public DropRcService dropRcService(RunStateStore store, StepRegistry registry, ProcessRunner runner, ReleaseEngine.SecretResolver secrets, ReleaseEngine engine, ExecutionMode mode, TargetProfile target, - @Value("${rm.staging.dir}") String stagingDir, @Value("${rm.state.dir}") String stateDir) { + LoopbackBoundary boundary, @Value("${rm.staging.dir}") String stagingDir, + @Value("${rm.state.dir}") String stateDir) { var svc = new DropRcService(store, registry, runner, Path.of(stagingDir).resolve("git/juneau"), Path.of(stateDir), secrets.nexus(), mode, engine::isArmed, target, engine::broadcaster); if (engine.mockNexusBaseUrl() != null) svc.setSafeNexus(NexusStagingClient.create(engine.mockNexusBaseUrl(), target.nexusProfileId(), - SAFE_PLACEHOLDER, SAFE_PLACEHOLDER)); + SAFE_PLACEHOLDER, SAFE_PLACEHOLDER, boundary.selfCallHeaders())); return svc; } diff --git a/src/main/java/org/apache/juneau/releng/engine/ReleaseEngine.java b/src/main/java/org/apache/juneau/releng/engine/ReleaseEngine.java index f01e343360..457eadc113 100644 --- a/src/main/java/org/apache/juneau/releng/engine/ReleaseEngine.java +++ b/src/main/java/org/apache/juneau/releng/engine/ReleaseEngine.java @@ -75,6 +75,7 @@ public class ReleaseEngine { // Loopback mock base (http://host:port/mock/nexus). Null in forTests so those keep secrets.nexus(). private String mockNexusBaseUrl; + private Map<String, String> loopbackHeaders = Map.of(); /** Everything the REST layer must provide to build a mutating StepContext. */ public interface SecretResolver { @@ -269,6 +270,17 @@ public class ReleaseEngine { return mockNexusBaseUrl; } + /** + * Headers the SAFE-mode Nexus client must present to get past the loopback write boundary, since the mock it + * talks to is mounted on this application's own port. Empty in {@link #forTests}, where no boundary is + * installed and the transport is a stub anyway. + * + * @see org.apache.juneau.rest.server.filter.LoopbackBoundary#selfCallHeaders() + */ + public void setLoopbackHeaders(Map<String, String> headers) { + this.loopbackHeaders = headers == null ? Map.of() : Map.copyOf(headers); + } + /** * The run's effective mode: persisted {@code rs.mode} (null → SAFE), capped so a SAFE box can never * execute LIVE even if on-disk state claims it. @@ -296,6 +308,25 @@ public class ReleaseEngine { * Arms {@code version} for LIVE mutation. Rejected unless the box is LIVE, this run is Actual (LIVE), * and {@code confirm} equals the required phrase {@code "<version> LIVE"}. Returns the outcome * message-bearing result. + * + * <p><b>Arming is an intent gate, and only that.</b> It establishes that a human meant to do something + * irreversible; it establishes nothing about who or what sent the request. The two questions are separate, and + * the second one is answered by + * {@link org.apache.juneau.rest.server.filter.LoopbackBoundary} — see {@code AppConfiguration}. + * + * <p>Specifically, <b>the confirm phrase is not a secret and carries no authenticity.</b> It is + * {@code "<version> LIVE"}, and the version is displayed on the very page an attacker would be reading, so any + * page in the operator's browser could derive it. Before the boundary existed, a hostile page could POST that + * phrase to {@code /arm} as a plain cross-origin form submission and then trigger a mutating step. What the + * phrase does buy is real but narrower than it looks: it makes an irreversible action require deliberate typing + * rather than a misplaced click, which is worth having, and it is worth being clear that this is all it is. + * + * <p><b>Do not respond to that by making the phrase harder to guess.</b> A longer or hidden phrase would not + * help. Anything the page must display so the operator can type it is readable by any script running in that + * browser, and anything the operator memorises instead gets written down. A secret shared with the attacker is + * not a secret, and dressing this gate up as authentication would obscure the fact that authentication is a + * separate control that has to exist on its own — which is what the boundary is. Keep this phrase exactly as + * hard to type as it needs to be to prevent an accident, and no harder. */ public synchronized StepResult arm(String version, String confirm) { if (mode != ExecutionMode.LIVE) @@ -516,7 +547,7 @@ public class ReleaseEngine { if (runMode == ExecutionMode.SAFE && mockNexusBaseUrl != null) { ctx.target = target.withNexusBaseUrl(mockNexusBaseUrl); ctx.nexus = NexusStagingClient.create(mockNexusBaseUrl, target.nexusProfileId(), "safe-placeholder", - "safe-placeholder"); + "safe-placeholder", loopbackHeaders); } else { ctx.target = target; ctx.nexus = secrets.nexus(); diff --git a/src/main/java/org/apache/juneau/releng/nexus/NexusStagingClient.java b/src/main/java/org/apache/juneau/releng/nexus/NexusStagingClient.java index 6885feaa1d..301ad96082 100644 --- a/src/main/java/org/apache/juneau/releng/nexus/NexusStagingClient.java +++ b/src/main/java/org/apache/juneau/releng/nexus/NexusStagingClient.java @@ -64,10 +64,26 @@ public class NexusStagingClient { * here. Under SAFE mode {@code baseUrl} is the in-app loopback mock and the credential is a placeholder. */ public static NexusStagingClient create(String baseUrl, String profileId, String username, String password) { + return create(baseUrl, profileId, username, password, Map.of()); + } + + /** + * As {@link #create(String, String, String, String)}, plus {@code extraHeaders} added to every request. + * + * <p> + * Exists for the SAFE-mode loopback mock. That mock is mounted on this application's own port, behind the + * {@link org.apache.juneau.rest.server.filter.LoopbackBoundary LoopbackBoundary}, which grants no exemption + * to a caller merely because it happens to be this process — so the close/drop/promote {@code POST}s below + * must present the same {@code Origin} and CSRF token the browser does. Pass + * {@link org.apache.juneau.rest.server.filter.LoopbackBoundary#selfCallHeaders() selfCallHeaders()} here. + * The real Nexus needs none of this and is given an empty map. + */ + public static NexusStagingClient create(String baseUrl, String profileId, String username, String password, + Map<String, String> extraHeaders) { var http = HttpClient.newHttpClient(); var basic = "Basic " + Base64.getEncoder().encodeToString((username + ":" + password).getBytes(StandardCharsets.UTF_8)); - return new NexusStagingClient(transport(http, basic, baseUrl), profileId); + return new NexusStagingClient(transport(http, basic, baseUrl, extraHeaders), profileId); } /** @@ -84,11 +100,13 @@ public class NexusStagingClient { return create(baseUrl, profileId, creds.username(), creds.password()); } - private static Transport transport(HttpClient http, String basic, String baseUrl) { + private static Transport transport(HttpClient http, String basic, String baseUrl, + Map<String, String> extraHeaders) { return (method, path, body) -> { try { var b = HttpRequest.newBuilder(URI.create(baseUrl + path)).header("Authorization", basic) .header("Accept", "application/json").header("Content-Type", "application/json"); + extraHeaders.forEach(b::header); var req = (body == null) ? b.method(method, HttpRequest.BodyPublishers.noBody()).build() : b.method(method, HttpRequest.BodyPublishers.ofString(body)).build(); var resp = http.send(req, HttpResponse.BodyHandlers.ofString()); diff --git a/src/main/java/org/apache/juneau/releng/rest/AdminRest.java b/src/main/java/org/apache/juneau/releng/rest/AdminRest.java index 6b0c439c69..e0754e238f 100644 --- a/src/main/java/org/apache/juneau/releng/rest/AdminRest.java +++ b/src/main/java/org/apache/juneau/releng/rest/AdminRest.java @@ -24,7 +24,6 @@ import org.apache.juneau.rest.server.RestGet; import org.apache.juneau.rest.server.servlet.BasicRestResource; import org.apache.juneau.rest.server.view.View; import org.apache.juneau.rest.server.view.freemarker.FreemarkerMixin; -import org.apache.juneau.rest.server.view.freemarker.FreemarkerView; import org.apache.juneau.rest.server.view.freemarker.FreemarkerViewRenderer; import org.apache.juneau.rest.server.view.freemarker.console.ConsoleFreemarkerMixin; import org.apache.juneau.rest.server.views.PageDef; @@ -32,6 +31,8 @@ import org.apache.juneau.rest.server.views.PageTable; import org.apache.juneau.rest.server.views.Tab; import org.apache.juneau.rest.server.views.ViewsMixin; +import jakarta.servlet.http.HttpServletRequest; + /** * Admin tab (TODO-399 Phase C dogfood): a single multi-tab page composing the app's existing * {@link ReleaseRest#releasesView() Releases} and {@link CredentialRest#credentialsView() Credentials} rich views @@ -43,7 +44,7 @@ import org.apache.juneau.rest.server.views.ViewsMixin; * definitions, and each child view's {@code dataUrl} stays absolute (its owning resource's own mount), so the * ajax data draws still hit {@link ReleaseRest#data()} / {@link CredentialRest#status()} exactly as they do from * the standalone Releases/Credentials pages. Per {@link PageTable}'s contract, the emitted per-view markup (marker - * table + VIEW_META sidecar) is byte-for-byte identical to what {@link ReleaseRest#page()} / + * table + VIEW_META sidecar) is byte-for-byte identical to what {@link ReleaseRest#page(HttpServletRequest)} / * {@code CredentialRest}'s own view would emit standalone — this resource only adds the tab-bar/panel shell * and the PAGE_META sidecar around them. */ @@ -76,9 +77,9 @@ public class AdminRest extends BasicRestResource { /** Human page — the composed tab/sub-tab page shell (emitted as trusted markup) + PAGE_META sidecar. */ @RestGet("/") - public View page() { + public View page(HttpServletRequest req) { var markup = HtmlSerializer.DEFAULT_SIMPLE_SQ.toString(PageTable.of(adminPage())); - return FreemarkerView.of("admin") + return ConsolePage.of("admin", req) .attr("pageTable", markup) .attr("viewsCssUrl", asset(ViewsMixin.VIEWS_CSS_PATH)) .attr("rendersJsUrl", asset(ViewsMixin.RENDERS_JS_PATH)) diff --git a/src/main/java/org/apache/juneau/releng/rest/ConsolePage.java b/src/main/java/org/apache/juneau/releng/rest/ConsolePage.java new file mode 100644 index 0000000000..ddf708fec3 --- /dev/null +++ b/src/main/java/org/apache/juneau/releng/rest/ConsolePage.java @@ -0,0 +1,60 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.juneau.releng.rest; + +import org.apache.juneau.rest.server.filter.LoopbackBoundaryFilter; +import org.apache.juneau.rest.server.view.freemarker.FreemarkerView; + +import jakarta.servlet.http.HttpServletRequest; + +/** + * Starts every console page's view with the CSRF token {@code base.ftlh} embeds. + * + * <p>The token is read from the request attribute {@link LoopbackBoundaryFilter} sets on requests it allowed + * through, rather than from an injected bean, so that a page can only carry a token if the boundary is actually + * installed in front of it. Wiring the two together this way means "we serve tokens" and "we check tokens" cannot + * drift apart into the worst combination — a page that hands out a token nothing validates. + * + * <p>Forgetting to route a new page through here is safe rather than dangerous, and loud rather than quiet: + * {@code base.ftlh} references {@code ${csrfToken}} with no FreeMarker default, so the page fails to render at + * once instead of silently shipping without a token. Even if it did render, its writes would be refused by the + * filter. The security decision belongs to the filter; this class only supplies the UI half. + */ +final class ConsolePage { + + private ConsolePage() {} + + /** + * The named template, seeded with {@code csrfToken}. + * + * <p>When the attribute is absent — no boundary filter ran in front of this request, as in a unit test that + * dispatches straight at the resource — the token renders empty rather than failing the render. An empty + * token is never less safe than no token: the only thing that reads it is the boundary's check, which + * refuses an empty value like any other wrong one. If no boundary is installed, there is nothing for a token + * to have protected in the first place. The page-side complaint lives in {@code csrf.js}, which logs when it + * finds the meta tag empty, so the condition is still visible where somebody would notice it. + * + * @param template The template name, relative to the configured base path. + * @param req The current request, carrying the boundary's token attribute. + * @return A view the caller adds its own page attributes to. + */ + static FreemarkerView of(String template, HttpServletRequest req) { + var token = req.getAttribute(LoopbackBoundaryFilter.TOKEN_ATTRIBUTE); + return FreemarkerView.of(template).attr("csrfToken", token == null ? "" : token); + } +} diff --git a/src/main/java/org/apache/juneau/releng/rest/CredentialRest.java b/src/main/java/org/apache/juneau/releng/rest/CredentialRest.java index 848c546d20..290a4b45a8 100644 --- a/src/main/java/org/apache/juneau/releng/rest/CredentialRest.java +++ b/src/main/java/org/apache/juneau/releng/rest/CredentialRest.java @@ -21,6 +21,7 @@ import java.util.List; import org.apache.juneau.commons.inject.Bean; import org.apache.juneau.http.Content; import org.apache.juneau.http.Path; +import org.apache.juneau.rest.server.Mutating; import org.apache.juneau.rest.server.Rest; import org.apache.juneau.rest.server.RestDelete; import org.apache.juneau.rest.server.RestGet; @@ -28,7 +29,6 @@ import org.apache.juneau.rest.server.RestPost; import org.apache.juneau.rest.server.servlet.BasicRestResource; import org.apache.juneau.rest.server.view.View; import org.apache.juneau.rest.server.view.freemarker.FreemarkerMixin; -import org.apache.juneau.rest.server.view.freemarker.FreemarkerView; import org.apache.juneau.rest.server.view.freemarker.FreemarkerViewRenderer; import org.apache.juneau.rest.server.view.freemarker.console.ConsoleFreemarkerMixin; import org.apache.juneau.rest.server.views.Column; @@ -40,8 +40,19 @@ import org.apache.juneau.releng.credential.CredentialService; import org.apache.juneau.releng.credential.CredentialStatus; import org.apache.juneau.releng.credential.Validator.ValidationResult; -/** Credentials tab: store + live-validate Apache/GPG/GitHub secrets. Never returns secret values. */ -@Rest(path = "/credentials", title = "Credentials", responseProcessors = FreemarkerViewRenderer.class) +import jakarta.servlet.http.HttpServletRequest; + +/** + * Credentials tab: store + live-validate Apache/GPG/GitHub secrets. Never returns secret values. + * + * <p>{@code disableContentParam} is set because Juneau's default allows a {@code POST} body to arrive in a + * {@code &content=} query parameter instead, and a secret that travels in a URL lands in browser history, in any + * access log, and in the {@code Referer} of the next request. {@code LoopbackBoundary} already refuses that shape + * from a hostile page (it carries no JSON content type), so this is not the attack control — it closes the accident + * of a developer, a curl line or a copied URL doing it. See {@code CredentialWriteVectorTest}. + */ +@Rest(path = "/credentials", title = "Credentials", responseProcessors = FreemarkerViewRenderer.class, + disableContentParam = "true") public class CredentialRest extends BasicRestResource { /** This resource's absolute mount (RootRest {@code /rest/*} + {@code /credentials}), used by {@link #credentialsView()}. */ @@ -85,8 +96,8 @@ public class CredentialRest extends BasicRestResource { /** Human page. */ @RestGet("/") - public View page() { - return FreemarkerView.of("credentials").attr("credentials", service.status()); + public View page(HttpServletRequest req) { + return ConsolePage.of("credentials", req).attr("credentials", service.status()); } /** JSON status for all credentials (no secrets). */ @@ -96,6 +107,7 @@ public class CredentialRest extends BasicRestResource { } /** Store/update a credential. Body: {account?, secret}. Apache/GPG send account (availid/keyId). */ + @Mutating("replaces a stored credential in the Keychain") @RestPost("/{name}") public CredentialStatus set(@Path("name") String name, @Content StoreRequest body) { service.store(name, body.account, body.secret); @@ -103,12 +115,14 @@ public class CredentialRest extends BasicRestResource { } /** Run the live validation. */ + @Mutating("caches a new validation verdict, and makes an authenticated call as the user") @RestPost("/{name}/validate") public ValidationResult validate(@Path("name") String name) { return service.validate(name); } /** Remove a credential from the Keychain. */ + @Mutating("deletes a stored credential from the Keychain") @RestDelete("/{name}") public CredentialStatus remove(@Path("name") String name) { service.delete(name); diff --git a/src/main/java/org/apache/juneau/releng/rest/HomeRest.java b/src/main/java/org/apache/juneau/releng/rest/HomeRest.java index eb6c115be6..d2a88198f7 100644 --- a/src/main/java/org/apache/juneau/releng/rest/HomeRest.java +++ b/src/main/java/org/apache/juneau/releng/rest/HomeRest.java @@ -23,10 +23,11 @@ import org.apache.juneau.rest.server.RestGet; import org.apache.juneau.rest.server.servlet.BasicRestResource; import org.apache.juneau.rest.server.view.View; import org.apache.juneau.rest.server.view.freemarker.FreemarkerMixin; -import org.apache.juneau.rest.server.view.freemarker.FreemarkerView; import org.apache.juneau.rest.server.view.freemarker.FreemarkerViewRenderer; import org.apache.juneau.rest.server.view.freemarker.console.ConsoleFreemarkerMixin; +import jakarta.servlet.http.HttpServletRequest; + /** Home tab: default landing page with basic usage instructions. */ @Rest(path = "/home", title = "Home", responseProcessors = FreemarkerViewRenderer.class) public class HomeRest extends BasicRestResource { @@ -40,7 +41,7 @@ public class HomeRest extends BasicRestResource { /** Human page. */ @RestGet("/") - public View page() { - return FreemarkerView.of("home"); + public View page(HttpServletRequest req) { + return ConsolePage.of("home", req); } } diff --git a/src/main/java/org/apache/juneau/releng/rest/ReleaseRest.java b/src/main/java/org/apache/juneau/releng/rest/ReleaseRest.java index 6b8879d7e8..33a9b58cb6 100644 --- a/src/main/java/org/apache/juneau/releng/rest/ReleaseRest.java +++ b/src/main/java/org/apache/juneau/releng/rest/ReleaseRest.java @@ -29,7 +29,6 @@ import org.apache.juneau.rest.server.converter.QueryableSettings; import org.apache.juneau.rest.server.servlet.BasicRestResource; import org.apache.juneau.rest.server.view.View; import org.apache.juneau.rest.server.view.freemarker.FreemarkerMixin; -import org.apache.juneau.rest.server.view.freemarker.FreemarkerView; import org.apache.juneau.rest.server.view.freemarker.FreemarkerViewRenderer; import org.apache.juneau.rest.server.view.freemarker.console.ConsoleFreemarkerMixin; import org.apache.juneau.rest.server.views.Column; @@ -42,12 +41,14 @@ import org.apache.juneau.rest.server.views.ViewsMixin; import org.apache.juneau.releng.release.Release; import org.apache.juneau.releng.release.ReleaseListService; +import jakarta.servlet.http.HttpServletRequest; + /** * Releases tab: server-rendered HTML page + DataTables server-side-processing data endpoint. * * <p> * Built on the {@code juneau-rest-server-views} rich-view toolkit: {@link #releasesView()} declares the typed - * {@link ViewDef} (columns + renderers + ribbon), {@link #page()} emits its {@link ViewTable} shell as trusted markup + * {@link ViewDef} (columns + renderers + ribbon), {@link #page(HttpServletRequest)} emits its {@link ViewTable} shell as trusted markup * into the FreeMarker template, and {@link #data()} serves the {@code DataTablesResults} envelope via * {@link ProtocolQueryable} + the view's {@link ViewDef#queryableSettings() queryable settings}. The four runtime * assets are served by the composed {@link ViewsMixin} at this resource's mount. @@ -118,9 +119,9 @@ public class ReleaseRest extends BasicRestResource { /** Human page — the rich-view table shell (emitted as trusted markup) + JSON sidecar, hydrated by the toolkit JS. */ @RestGet("/") - public View page() { + public View page(HttpServletRequest req) { var markup = HtmlSerializer.DEFAULT_SIMPLE_SQ.toString(ViewTable.of(releasesView())); - return FreemarkerView.of("releases") + return ConsolePage.of("releases", req) .attr("viewTable", markup) .attr("viewsCssUrl", asset(ViewsMixin.VIEWS_CSS_PATH)) .attr("rendersJsUrl", asset(ViewsMixin.RENDERS_JS_PATH)) @@ -155,9 +156,9 @@ public class ReleaseRest extends BasicRestResource { * part of the path so a future multi-RC history view doesn't need a URL-breaking change. */ @RestGet("/{version}/{rc}") - public View detail(@Path("version") String version, @Path("rc") String rc) { + public View detail(@Path("version") String version, @Path("rc") String rc, HttpServletRequest req) { var release = findByVersion(version); - return FreemarkerView.of("release-detail").attr("release", release).attr("rc", rc); + return ConsolePage.of("release-detail", req).attr("release", release).attr("rc", rc); } private Release findByVersion(String version) { diff --git a/src/main/java/org/apache/juneau/releng/rest/ReleaseRunRest.java b/src/main/java/org/apache/juneau/releng/rest/ReleaseRunRest.java index fd5deb6a12..366337ac64 100644 --- a/src/main/java/org/apache/juneau/releng/rest/ReleaseRunRest.java +++ b/src/main/java/org/apache/juneau/releng/rest/ReleaseRunRest.java @@ -23,13 +23,13 @@ import org.apache.juneau.http.Content; import org.apache.juneau.http.Path; import org.apache.juneau.http.response.Conflict; import org.apache.juneau.http.response.NotFound; +import org.apache.juneau.rest.server.Mutating; import org.apache.juneau.rest.server.Rest; import org.apache.juneau.rest.server.RestGet; import org.apache.juneau.rest.server.RestPost; import org.apache.juneau.rest.server.servlet.BasicRestResource; import org.apache.juneau.rest.server.view.View; import org.apache.juneau.rest.server.view.freemarker.FreemarkerMixin; -import org.apache.juneau.rest.server.view.freemarker.FreemarkerView; import org.apache.juneau.rest.server.view.freemarker.FreemarkerViewRenderer; import org.apache.juneau.rest.server.view.freemarker.console.ConsoleFreemarkerMixin; import org.apache.juneau.releng.engine.DropRcService; @@ -39,8 +39,18 @@ import org.apache.juneau.releng.engine.ReleaseEngine; import org.apache.juneau.releng.engine.RunState; import org.apache.juneau.releng.engine.StepResult; -/** New Release tab: pipeline control panel (View) plus JSON run/step/vote/drop-RC actions. */ -@Rest(path = "/runs", title = "New Release", responseProcessors = FreemarkerViewRenderer.class) +import jakarta.servlet.http.HttpServletRequest; + +/** + * New Release tab: pipeline control panel (View) plus JSON run/step/vote/drop-RC actions. + * + * <p>{@code disableContentParam} is set for the reason given on {@code CredentialRest}: Juneau's default lets a + * {@code POST} body arrive in a {@code &content=} query parameter, which puts the arm confirmation phrase and every + * other action payload into browser history and access logs. The boundary refuses that shape from a hostile page; + * this closes the accidental use of it. + */ +@Rest(path = "/runs", title = "New Release", responseProcessors = FreemarkerViewRenderer.class, + disableContentParam = "true") public class ReleaseRunRest extends BasicRestResource { private final ReleaseEngine engine; @@ -60,11 +70,11 @@ public class ReleaseRunRest extends BasicRestResource { /** Human page — the pipeline control panel for the active run (or an empty start form). */ @RestGet("/") - public View page() { + public View page(HttpServletRequest req) { var liveCapable = engine.mode() == ExecutionMode.LIVE; var active = engine.displayRun().orElse(null); var runMode = active == null ? ExecutionMode.SAFE : engine.effectiveMode(active); - var view = FreemarkerView.of("new-release").attr("steps", engine.registry().steps()) + var view = ConsolePage.of("new-release", req).attr("steps", engine.registry().steps()) .attr("mode", runMode.name()).attr("appMode", engine.mode().name()) .attr("liveCapable", Boolean.valueOf(liveCapable)); // FreemarkerView.attr() rejects null values by design; the template only checks run?? @@ -86,6 +96,7 @@ public class ReleaseRunRest extends BasicRestResource { * capped to SAFE unless the box was started with {@code rm.mode=live}. Rejects a second concurrent run * with 409. */ + @Mutating("creates a run and writes its state to disk") @RestPost("/") public RunState start(@Content StartRequest body) { try { @@ -103,6 +114,7 @@ public class ReleaseRunRest extends BasicRestResource { * {@code knownIssues}, {@code acknowledgements}) so they can be edited before each email is composed. * Returns the updated run. */ + @Mutating("updates the run's persisted narrative fields") @RestPost("/{version}/details") public RunState details(@Path("version") String version, @Content DetailsRequest body) { requireRun(version); @@ -110,6 +122,9 @@ public class ReleaseRunRest extends BasicRestResource { return engine.updateDetails(version, b.releaseSummary, b.highlights, b.knownIssues, b.acknowledgements); } + // No @Mutating: a preview is a dry run by construction and writes nothing. The annotation is a claim about + // effects, so putting it here to be "safe" would be a false one -- and would make the two preview endpoints + // indistinguishable from the apply endpoints they exist to be safer than. @RestPost("/{version}/steps/{stepId}/preview") public Preview preview(@Path("version") String version, @Path("stepId") String stepId, @Content Map<String, String> form) { @@ -117,6 +132,7 @@ public class ReleaseRunRest extends BasicRestResource { return engine.preview(version, stepId, form == null ? Map.of() : form); } + @Mutating("executes a release step; in LIVE mode this mutates git, SVN, Nexus, GitHub or mailing lists") @RestPost("/{version}/steps/{stepId}/apply") public StepResult apply(@Path("version") String version, @Path("stepId") String stepId, @Content Map<String, String> form) { @@ -136,6 +152,7 @@ public class ReleaseRunRest extends BasicRestResource { * history. The UI picks the button label from the step's current status; the engine doesn't care which * label was clicked. */ + @Mutating("re-executes a release step, overwriting its status and log in place") @RestPost("/{version}/steps/{stepId}/resume") public StepResult resume(@Path("version") String version, @Path("stepId") String stepId, @Content Map<String, String> form) { @@ -143,6 +160,7 @@ public class ReleaseRunRest extends BasicRestResource { return engine.apply(version, stepId, form == null ? Map.of() : form); } + @Mutating("marks a step skipped in the persisted run state") @RestPost("/{version}/steps/{stepId}/skip") public StepResult skip(@Path("version") String version, @Path("stepId") String stepId) { requireRun(version); @@ -153,7 +171,13 @@ public class ReleaseRunRest extends BasicRestResource { * Arm this run for LIVE mutation. Requires a typed confirm phrase ({@code "<version> LIVE"}) and is * rejected unless the box is LIVE and this run is Actual (LIVE). Arming is in-memory on the engine and * drops on any restart. + * + * <p>This is an <b>intent</b> gate: the confirm phrase shows a human deliberately typed something, and is not + * a secret and not authentication — it is derivable from the page it is typed on. Whether the request came from + * a page this application served is a separate question, answered by the loopback boundary in front of every + * endpoint. See {@link org.apache.juneau.releng.engine.ReleaseEngine#arm(String, String)}. */ + @Mutating("arms the run for irreversible LIVE mutation") @RestPost("/{version}/arm") public StepResult arm(@Path("version") String version, @Content ArmRequest body) { requireRun(version); @@ -161,6 +185,7 @@ public class ReleaseRunRest extends BasicRestResource { } /** Advance a review-gate step held in {@code AWAITING_REVIEW} once the human has confirmed the read-only work. */ + @Mutating("advances a held review-gate step") @RestPost("/{version}/steps/{stepId}/confirm-review") public StepResult confirmReview(@Path("version") String version, @Path("stepId") String stepId) { requireRun(version); @@ -168,6 +193,7 @@ public class ReleaseRunRest extends BasicRestResource { } /** Record the vote outcome; 'rejected' triggers Drop-RC. */ + @Mutating("records the vote outcome and runs the tally step") @RestPost("/{version}/vote-result") public StepResult voteResult(@Path("version") String version, @Content VoteResultRequest body) { requireRun(version); @@ -182,6 +208,7 @@ public class ReleaseRunRest extends BasicRestResource { return dropRc.preview(version); } + @Mutating("drops the release candidate from Nexus and dist SVN, and bumps the RC number") @RestPost("/{version}/drop-rc/apply") public StepResult dropRcApply(@Path("version") String version, @Content DropRcRequest body) { var rs = requireRun(version); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index b23e2978ff..b7bc9003a1 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -24,4 +24,18 @@ [email protected] rm.slack.webhook= # Execution mode: safe (default) simulates all mutations + routes Nexus to the in-app loopback mock; # live executes real mutations (and still requires per-run arming at the guard chokepoint). +# +# safe being the default is an OPERATIONAL SAFETY default, not a security control. It exists so that starting the +# app, exploring it, or running it in a test cannot mutate anything public by accident. +# +# It is worth recording that until the loopback write boundary landed (see AppConfiguration), this default was in +# fact doing real security work, and was the only thing standing between a hostile page in the operator's browser +# and a published release: that page could derive the "<version> LIVE" confirm phrase from the app's own page, POST +# it to /arm cross-origin, and trigger a mutating step. Nothing rejected the request; the box simply had to have +# been started in live mode. +# +# That is exactly why it was never adequate as a security control. Its protection evaporates the moment someone +# starts the app with rm.mode=live -- which is to say, during an actual release, when the consequences of a forged +# request are at their worst. A control that is present only when it does not matter is not a control. The +# boundary is what closes that gap, and it applies in both modes. rm.mode=safe diff --git a/src/main/resources/static/js/csrf.js b/src/main/resources/static/js/csrf.js new file mode 100644 index 0000000000..8a4c858c07 --- /dev/null +++ b/src/main/resources/static/js/csrf.js @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/* + * Attaches this page's CSRF token to every state-changing fetch. + * + * The server side of this is LoopbackBoundary, which refuses any non-GET request that does not present the + * token. Rather than adding a header at each of the app's fetch() call sites -- where the next one added would + * omit it, and would fail at runtime in whichever corner of the UI nobody clicked before shipping -- this wraps + * window.fetch once, so carrying the token is the default and opting out is not expressible. + * + * Loaded first in base.ftlh's <head>, before any script that might issue a request. + * + * The token is read from the <meta> tag the server rendered. It is deliberately never written to + * document.cookie: cookies are scoped by host and ignore the port, so a cookie-borne token could be planted by + * any page served from any other port on this host. See SynchronizerToken's javadoc. + */ +(function () { + 'use strict'; + + var meta = document.querySelector('meta[name="csrf-token"]'); + var token = meta && meta.getAttribute('content'); + if (!token) { + // No token means the page was not served through ConsolePage, or the boundary filter is not installed. + // Say so once, loudly: every write from this page is about to be refused, and a console message is far + // easier to act on than a screenful of unexplained 403s. + if (window.console && console.error) { + console.error('No csrf-token meta tag; state-changing requests from this page will be refused.'); + } + return; + } + + // 'simple' request methods per fetch: these never need the token, and adding it to a cross-origin GET would + // hand it to whatever was fetched. Anything else -- including a method this list does not know -- gets it. + var SAFE = { GET: 1, HEAD: 1, OPTIONS: 1, TRACE: 1 }; + + // Only same-origin requests get the token. A relative URL is same-origin by construction; an absolute one is + // compared against location.origin. This keeps the secret from leaking to a third-party endpoint if some + // future code fetches one. + function isSameOrigin(url) { + try { + return new URL(url, window.location.href).origin === window.location.origin; + } catch (e) { + return false; + } + } + + var nativeFetch = window.fetch.bind(window); + + window.fetch = function (resource, init) { + var opts = init || {}; + var method = (opts.method || (resource && resource.method) || 'GET').toUpperCase(); + var url = (resource && resource.url) || resource; + + if (SAFE[method] || !isSameOrigin(url)) { + return nativeFetch(resource, init); + } + + // Headers may arrive as a Headers instance, an array of pairs, or a plain object; normalizing through + // Headers handles all three without caring which the caller used. + var headers = new Headers((opts.headers) || (resource && resource.headers) || undefined); + headers.set('X-Csrf-Token', token); + + // The boundary also requires a JSON content type on writes, which is what rules out the form-encoded + // shapes a cross-origin <form> can submit with no preflight. It is required on every write, including a + // bodiless one: a POST with no body and no Content-Type is itself a no-preflight shape, so the server + // cannot exempt it, and the client must therefore supply the header even when it has nothing to send. + if (!headers.has('Content-Type')) { + headers.set('Content-Type', 'application/json'); + } + + var merged = {}; + for (var k in opts) { + if (Object.prototype.hasOwnProperty.call(opts, k)) { + merged[k] = opts[k]; + } + } + merged.headers = headers; + return nativeFetch(resource, merged); + }; + + // Exposed for any code that must build a request by hand (e.g. XMLHttpRequest, or a fetch it deliberately + // routes around the wrapper). + window.RmCsrf = { + token: token, + header: 'X-Csrf-Token' + }; +})(); diff --git a/src/main/resources/templates/base.ftlh b/src/main/resources/templates/base.ftlh index e59bac0ac7..13b587e22f 100644 --- a/src/main/resources/templates/base.ftlh +++ b/src/main/resources/templates/base.ftlh @@ -27,6 +27,18 @@ <head> <meta charset="utf-8"> <title>Apache Juneau · Release Manager</title> + <#-- The server-held CSRF token for this boot, required back on every state-changing request by the loopback + write boundary (LoopbackBoundary). Supplied by ConsolePage.of(...) from the request attribute the + boundary's filter sets; referenced with no FreeMarker default on purpose, so a page that forgets to go + through ConsolePage fails to render instead of quietly shipping a UI whose writes all 403. + + This is a synchronizer token, NOT a double-submit cookie. Do not "simplify" it into one: cookies ignore + the port, so any page on any other localhost:* could plant a value this app would read back and accept + as its own. csrf.js reads it from here; nothing writes it to document.cookie. --> + <meta name="csrf-token" content="${csrfToken}"> + <#-- Must load before any other script: it wraps window.fetch so every non-GET request carries the token + above, rather than leaving each call site to remember. --> + <script src="/js/csrf.js"></script> <link rel="icon" type="image/svg+xml" href="/img/oakleaf.svg"> <#if activeTab?? && (activeTab == 'releases' || activeTab == 'admin')> <link rel="stylesheet" href="/datatables/dataTables.dataTables.min.css"> diff --git a/src/test/java/org/apache/juneau/releng/rest/CredentialWriteVectorTest.java b/src/test/java/org/apache/juneau/releng/rest/CredentialWriteVectorTest.java new file mode 100644 index 0000000000..79b710413e --- /dev/null +++ b/src/test/java/org/apache/juneau/releng/rest/CredentialWriteVectorTest.java @@ -0,0 +1,211 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.juneau.releng.rest; + +import static org.junit.jupiter.api.Assertions.*; + +import java.nio.file.*; +import java.util.*; + +import org.apache.juneau.commons.inject.StackOverlay; +import org.apache.juneau.commons.secret.*; +import org.apache.juneau.releng.credential.*; +import org.apache.juneau.rest.mock.MockRestClient; +import org.apache.juneau.rest.server.filter.*; +import org.junit.jupiter.api.*; +import org.junit.jupiter.api.io.*; + +/** + * The credential-overwrite vector, reproduced and then closed. + * + * <p> + * The finding this pins down (recorded as F1 in {@code .work/specs/2026-08-18-console-credentials-surface.md}) was + * established by reading framework defaults rather than by issuing a request: {@code CredentialRest} extends + * {@code BasicRestResource}, whose default parser list includes {@code UrlEncodingParser} and + * {@code PlainTextParser}, and {@code disableContentParam} defaults to {@code false}. If that reading is right, + * then a plain cross-origin {@code <form method="POST">} in any page the operator has open can replace a stored + * credential — no JavaScript, and no CORS preflight, because form-encoded is one of the three content types a + * browser may send cross-origin without one. + * + * <p> + * Group a reproduces it. The store is an {@link InMemorySecretStore} and never the Keychain, so running these tests + * cannot touch a real credential — but everything between the request and the store is the real thing: the + * real resource class, its real default parser list, real content negotiation and real {@code @Content} binding. + * The vector is the framework's defaults doing exactly what they are configured to do, which is why it holds for + * any application on {@code BasicUniversalConfig} and not only this one. + * + * <p> + * Group b closes it, at the boundary rather than at the resource. {@link MockRestClient} dispatches into a + * {@code RestContext} directly and so does not run the servlet filter chain — which is the point: group a's + * requests reach the handler precisely because nothing stands in front of it, and the fix is to put something + * there. Group b therefore asserts the refusal at {@link LoopbackBoundary}, where the decision is actually made. + * + * @see LoopbackBoundary + */ +class CredentialWriteVectorTest { + + private static final String KEYCHAIN_FREE_ACCOUNT = "jdoe"; + + private InMemorySecretStore apacheStore; + private CredentialService service; + + @BeforeEach + void setUp(@TempDir Path stateDir) { + apacheStore = new InMemorySecretStore(); + var stores = new EnumMap<CredentialSpec,SecretStore>(CredentialSpec.class); + for (var spec : CredentialSpec.values()) + stores.put(spec, spec == CredentialSpec.APACHE_LDAP ? apacheStore : new InMemorySecretStore()); + service = new CredentialService(stores, new EnumMap<>(CredentialSpec.class), new AccountStore(stateDir)); + service.store("apache", KEYCHAIN_FREE_ACCOUNT, "the-real-password"); + } + + @SuppressWarnings("resource") // Caller closes via try-with-resources; MockRestClient caches RestContext per class, so opt out with a fresh StackOverlay. + private MockRestClient client() { + return MockRestClient.builder(new CredentialRest(service)).overridingBeanStore(new StackOverlay()).build(); + } + + private String storedSecret() { + return apacheStore.find(KEYCHAIN_FREE_ACCOUNT).map(String::new).orElse(null); + } + + // ----------------------------------------------------------------------------------------------------------- + // a - the vector, reproduced through the real resource + // ----------------------------------------------------------------------------------------------------------- + + @Test + void formEncodedPostOverwritesAStoredCredential() throws Exception { + // The whole finding in one assertion. This is the body a cross-origin <form> submits, with the content + // type a browser sets for it, and it lands in the Keychain slot in a real deployment. + try (var c = client(); + var res = c.post("/apache").header("Content-Type", "application/x-www-form-urlencoded") + .bodyString("account=" + KEYCHAIN_FREE_ACCOUNT + "&secret=hijacked").run()) { + assertEquals(200, res.getStatusCode()); + } + assertEquals("hijacked", storedSecret(), "form-encoded POST did not reach the store"); + } + + @Test + void plainTextPostOverwritesAStoredCredential() throws Exception { + // text/plain is the second of the three no-preflight content types, and PlainTextParser is in the same + // default list. Included because closing only the urlencoded shape would leave the vector open. + try (var c = client(); + var res = c.post("/apache").header("Content-Type", "text/plain") + .bodyString("{\"account\":\"" + KEYCHAIN_FREE_ACCOUNT + "\",\"secret\":\"hijacked-plain\"}").run()) { + assertEquals(200, res.getStatusCode()); + } + assertEquals("hijacked-plain", storedSecret(), "text/plain POST did not reach the store"); + } + + @Test + void contentQueryParameterIsRefusedByTheResource() throws Exception { + // The second half of F1, asserted closed rather than reproduced. disableContentParam defaults to false, so + // the body can travel in the URL instead; CredentialRest now sets it to "true", and this fails if that is + // removed. + // + // Worth a test of its own rather than folding into the form vector, for two reasons. The attacker sends no + // body and therefore no content type at all, so a check that only inspected the content type of a present + // body would wave it through. And a URL-borne secret has a second life the form shape does not: browser + // history, access logs, and the Referer of whatever the page loads next. + // + // The payload is UON, not JSON, because that is what the server would actually parse -- RestRequest's + // constructor rewrites the content type to UON's own and hands the parameter to UonParser. JSON here would + // be refused as malformed and this test would pass while proving nothing about disableContentParam. It is + // asserted on the store, not only the status, for the same reason. + // + // This is defence in depth and not the primary control: the boundary refuses this shape from a hostile + // page anyway (group b). What it buys is that a developer or a copied URL cannot use it either. + try (var c = client(); + var res = c.post("/apache?content=" + java.net.URLEncoder.encode( + "(account=" + KEYCHAIN_FREE_ACCOUNT + ",secret=hijacked-via-url)", + java.nio.charset.StandardCharsets.UTF_8)).run()) { + assertNotEquals(200, res.getStatusCode(), "content= parameter was still honoured"); + } + assertEquals("the-real-password", storedSecret(), "content= parameter still reached the store"); + } + + // ----------------------------------------------------------------------------------------------------------- + // b - closed by the boundary + // ----------------------------------------------------------------------------------------------------------- + + private static LoopbackBoundary boundary() { + return LoopbackBoundary.create().authority("127.0.0.1:8790").token(SynchronizerToken.of("t0ken")).build(); + } + + /** + * The content type is stubbed on {@code getContentType()} and not merely as a header, because that is what the + * boundary reads. Stubbing only the header leaves it {@code null}, and every write then fails the content-type + * check for the wrong reason -- which would make the refusals below pass vacuously. + */ + private static jakarta.servlet.http.HttpServletRequest req(String method, String contentType, Map<String,String> headers) { + var r = org.mockito.Mockito.mock(jakarta.servlet.http.HttpServletRequest.class); + org.mockito.Mockito.when(r.getMethod()).thenReturn(method); + org.mockito.Mockito.when(r.getContentType()).thenReturn(contentType); + headers.forEach((k, v) -> org.mockito.Mockito.when(r.getHeader(k)).thenReturn(v)); + return r; + } + + @Test + void theBoundaryRefusesTheFormEncodedPost() { + // The exact request from group a, as it would arrive from a hostile page: refused on content type, so it + // never reaches CredentialRest and the store is never opened. + var res = boundary().check(req("POST", "application/x-www-form-urlencoded", Map.of( + "Host", "127.0.0.1:8790", + "Origin", "http://evil.example";))); + assertFalse(res.isAllowed()); + assertEquals(403, res.status(), "foreign Origin is caught before the content type"); + } + + @Test + void theBoundaryRefusesTheFormEncodedPostEvenFromOurOwnOrigin() { + // With the origin corrected, the content-type check is what stops it. This is the assertion that the + // no-preflight form shape is unreachable, rather than merely that this particular attacker got the Origin + // wrong. + var res = boundary().check(req("POST", "application/x-www-form-urlencoded", Map.of( + "Host", "127.0.0.1:8790", + "Origin", "http://127.0.0.1:8790";, + "Sec-Fetch-Site", "same-origin", + "X-Csrf-Token", "t0ken"))); + assertFalse(res.isAllowed()); + assertEquals(415, res.status()); + } + + @Test + void theBoundaryRefusesTheContentQueryParameterPost() { + // The content= variant carries no body and therefore no content type, which is itself a no-preflight + // shape. It has to be refused on that absence rather than exempted for it -- and note the server would + // otherwise rewrite the content type to UON itself, so there is nothing downstream to catch it either. + var res = boundary().check(req("POST", null, Map.of( + "Host", "127.0.0.1:8790", + "Origin", "http://127.0.0.1:8790";, + "Sec-Fetch-Site", "same-origin", + "X-Csrf-Token", "t0ken"))); + assertFalse(res.isAllowed()); + assertEquals(415, res.status()); + } + + @Test + void theBoundaryAllowsTheLegitimateJsonPost() { + // The other direction, so this class cannot pass by refusing everything. + var res = boundary().check(req("POST", "application/json", Map.of( + "Host", "127.0.0.1:8790", + "Origin", "http://127.0.0.1:8790";, + "Sec-Fetch-Site", "same-origin", + "X-Csrf-Token", "t0ken"))); + assertTrue(res.isAllowed()); + } +} diff --git a/src/test/java/org/apache/juneau/releng/rest/ReleaseRestTest.java b/src/test/java/org/apache/juneau/releng/rest/ReleaseRestTest.java index 3a32235872..3b40717201 100644 --- a/src/test/java/org/apache/juneau/releng/rest/ReleaseRestTest.java +++ b/src/test/java/org/apache/juneau/releng/rest/ReleaseRestTest.java @@ -18,6 +18,7 @@ package org.apache.juneau.releng.rest; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; import java.util.List; import org.apache.juneau.commons.inject.StackOverlay; @@ -27,6 +28,8 @@ import org.apache.juneau.releng.release.ReleaseListService; import org.apache.juneau.rest.mock.MockRestClient; import org.junit.jupiter.api.Test; +import jakarta.servlet.http.HttpServletRequest; + class ReleaseRestTest { private ReleaseRest rest(List<Release> releases) { @@ -50,17 +53,25 @@ class ReleaseRestTest { return MockRestClient.builder(rest).overridingBeanStore(new StackOverlay()).build(); } + /** + * A request with no loopback-boundary token attribute, which is what a direct call (no servlet filter in the + * path) sees. {@code ConsolePage} renders the token empty in that case rather than failing. + */ + private static HttpServletRequest req() { + return mock(HttpServletRequest.class); + } + @Test void detailReturnsAViewCarryingTheMatchingRelease() { var rest = rest(List.of(release("9.2.1", "RELEASED"))); - var view = rest.detail("9.2.1", "1"); + var view = rest.detail("9.2.1", "1", req()); assertNotNull(view); } @Test void detailForAnUnknownVersionIs404() { var rest = rest(List.of(release("9.2.1", "RELEASED"))); - var ex = assertThrows(NotFound.class, () -> rest.detail("9.9.9", "1")); + var ex = assertThrows(NotFound.class, () -> rest.detail("9.9.9", "1", req())); assertEquals(404, ex.getStatusCode()); }
