Feat/frontend owasp security test - #17
Merged
Merged
Conversation
Closes the two advisories reported by `npm audit --omit=dev` for 21.2.18: - GHSA-jj27-h5hq-8x99 (XSS via i18n event-handler attributes) - GHSA-jhpw-976m-542j (HttpTransferCache cross-request response reuse) Neither is exploitable here (the app uses Transloco instead of Angular i18n and has no SSR/hydration), but the fix is a patch release and `npm audit --omit=dev` now reports 0 vulnerabilities instead of 7 high. The ranges are raised from ^21.2.0 to ^21.2.19 so the patched version is the documented floor. The @angular/* lockfile entries had to be re-resolved as a group: npm anchors peer resolution on the installed tree, so updating them one by one deadlocks on `peerOptional @angular/animations@21.2.18 from @angular/platform-browser@21.2.18`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… text
Transloco re-scans the result of a placeholder substitution for further
placeholders: DefaultTranspiler.transpile() loops over the already
substituted string and `interpolationMatcher` is a getter returning a
fresh RegExp with lastIndex = 0 on every access. A parameter value that
itself contains `{{...}}` is therefore resolved a second time.
Reproduced against the installed Transloco 8.4.0:
agreement = "{{legalDescription.unrestricted}}"
-> splices an unrelated sentence into the legally binding text
agreement = "{{constructor}}"
-> "function Object() { [native code] }"
agreement = "{{agreement}}"
-> substitutes itself, the string never changes, the while loop
never terminates and the browser tab freezes
Constraint values are not only produced by the UI (where a dropdown and
a constant cap them) but also read from GET /v1/policies/:id. An HTTP
response is input; the Constraint type is only a compile-time promise.
Fixed by only forwarding values that provably come from the metadata
registry: `agreement` is compared against FRAMEWORK_AGREEMENT_VALUE,
use-case labels are looked up in USE_CASE_OPTIONS instead of building
the i18n key from the id, and formatDate() no longer falls back to the
raw value. Anything else renders as "—".
Tests assert the invariant directly (no `{{` or `}}` in any key or
parameter handed to translate) and fail without the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`paramMap.get('id')` is input, not a trusted identifier. Angular's
DefaultUrlSerializer decodes %2F and %2E after splitting the path into
segments, so a route parameter can contain slashes and dot segments.
Verified against the installed @angular/router:
/policies/..%2F..%2Factuator%2Fenv
-> segments ["policies", "../../actuator/env"]
/policies/%2E%2E%2F%2E%2E%2Fadmin/edit
-> segments ["policies", "../../admin", "edit"] (matches :id/edit)
The service interpolated that straight into a template string, and the
browser normalises the result before sending:
new URL('/api/v1/policies/../../actuator/env', origin).pathname
-> '/api/actuator/env'
So a crafted link made the app issue a same-origin request to an
arbitrary /api endpoint with the user's credentials — and on the edit
route a single click on "Save" turned that into a PUT. Being same-origin,
neither CORS nor SameSite applies.
All three id-bearing paths now go through a shared resourceUrl() helper
that applies encodeURIComponent. Tests assert the invariant (the
resolved path stays below <backendUrl>/v1/policies/) and fail without
the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CONSTRAINT_METADATA is typed as Record<ConstraintType, ConstraintMetadata>,
which only holds at compile time. Constraints are also read from
GET /v1/policies/:id, so the type is an unverified claim about runtime
data. For an unrecognised type the index access returns undefined and
the following property read throws mid-render:
TypeError: Cannot read properties of undefined (reading 'labelKey')
TypeError: Cannot read properties of undefined (reading 'allowedIn')
A single stored policy with {"type":"FOO"} therefore left the detail
page broken for every user until the record was removed.
Adds isKnownConstraintType() (hasOwnProperty, so inherited Object
members like `constructor` are not mistaken for registry entries) and
keepKnownConstraints(). Unknown types are now dropped at the two load
boundaries with a warning to the user, buildLegalClauses() skips them
instead of aborting the whole legal text, and validatePolicyDraft()
reports them as a validation error.
New i18n keys in de.json and en.json:
validation.constraintUnknownType, policyDetail.notifications.
unknownConstraints, policyEditor.notifications.unknownConstraints.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
validatePolicyDraft() only checked that the id was non-empty and at most 200 characters. The value is taken over verbatim by policyToOdrl() as the JSON-LD @id of the emitted PolicyDefinition. @id is an identifier, not a label. An unconstrained string can therefore produce an absolute IRI (https://w3id.org/.../another-policy) or a blank node reference (_:b0) and point the document at a different resource than the one being edited. Adds a slug pattern (letter or digit first, then also dot, underscore, hyphen) which covers the documented format "policy.use-case-quality-assurance" and every id in the mock dataset and E2E specs. Reported as validation.policyIdInvalidChars, added to de.json and en.json. The check runs after the required/too-long checks so an empty id still reports policyIdRequired rather than a confusing charset error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The legal text is generated client-side in PolicyBuilderComponent.submit() and, per the backend contract documented on CreatePolicyRequest.legalText, persisted and returned unchanged. The detail page never read it: it re-derived the text from the constraints on every render. So the persisted, legally binding text and the text shown to a reviewer are two different things, and any divergence between them was invisible in this UI by construction — not only for tampered data, but also when the derivation logic or de.json changes and older records keep the old wording in the database. Policy did not even model legalText, so the page could not have read it. Added as an optional field (older records have none), and the MirageJS mock now stores and returns it so the behaviour exists in mock mode too. The comparison lives in hasDivergingLegalText() rather than in the component: it is a pure transformation, which is where this project puts them, and it makes the case unit-testable. E2E covers the good case (a policy saved through the UI must never warn), which is the regression guard for the derivation logic. The tampering case cannot be staged honestly in Cypress — MirageJS replaces XMLHttpRequest inside the browser, so neither cy.intercept nor an XHR patch reaches the request — and is covered by unit tests instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All six error callbacks were declared as `error: () => …`, so the
HttpErrorResponse was not even accepted as a parameter. Status code,
method and target were discarded, and every failure produced the same
snackbar: 401, 403, 404 and 500 were indistinguishable both for the user
and for anyone debugging. Repeated failing requests left no client-side
trace at all.
Two pieces, deliberately separate:
- httpErrorInterceptor logs one line per failed request with method,
url and status. Metadata only — never the request or response body,
which can carry policy ids and legal text.
- httpErrorMessageKey() picks the i18n key. The caller's contextual
message stays in charge of what it describes well (404 and other 4xx);
it is overridden only where the cause itself is the more useful
information: status 0 (no response at all), 401, 403, 409 and 5xx.
Keeping the message in the caller rather than the interceptor means the
user still gets exactly one notification.
New keys httpError.{offline,unauthorized,forbidden,conflict,server} in
de.json and en.json.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`@if (useMocks)` only controls rendering; what ends up in the bundle is
decided by the import statements. App imported MockDataSwitcherComponent
statically, which pulls in @mocks/mock-data-config and, through it,
@mocks/data/policies/mocked-policies.
Verified in dist/frontend/browser/main-*.js before this change: the full
mock dataset, the German developer labels ("Wenige Policies") and
var V=new ae(localStorage.getItem("mock-policy-mode")||"few")
That last line runs at module evaluation time — before
bootstrapApplication(), so the catch in main.ts would not cover it. Where
site storage is blocked (enterprise policy, Safari lockdown, embedded
iframe) touching localStorage throws SecurityError and the app would fail
to start with a blank page.
The replacement targets a barrel file rather than the component. Angular
keeps the templateUrl-to-component-path association across
fileReplacements, so replacing a component with an external template
leaves the AOT compiler checking the original template against the stub
class and the build fails with TS2339. A barrel has no template.
Independently, the localStorage access is now wrapped: reading falls back
to the default mode, writing degrades to session-only.
Initial bundle 508.45 kB -> 499.32 kB, and none of the strings above
appear anywhere in the production output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Angular's font inlining only solved half of this. It fetches the Google stylesheet at build time and inlines it as a <style> block, but the `src: url(...)` inside still pointed at fonts.gstatic.com. Verified in the previous build output: 21 @font-face rules referencing 11 distinct gstatic URLs, plus both preconnect links left in place. Every page view therefore still sent the user's IP to Google (GDPR Art. 44 ff.). The build also had a hard, uncached network dependency on the font CDN. With the network blocked it aborted outright: ✖ Building... [FAILED: Inlining of fonts failed ... ECONNREFUSED] and the fetched CSS was inlined into index.html verbatim, so whoever controls that response controls CSS in the app shell. Both are gone now: the five woff2 files ship with the app, and _fonts.scss is generated from the Google CSS so weights, styles and unicode-range are unchanged and rendering stays identical. Only the cyrillic, cyrillic-ext and vietnamese subsets are dropped — the app is de/en only. Montserrat normal is a variable font covering 300/400/600, so three rules share one file, exactly as Google serves it. The .material-icons class came from that external stylesheet too and is now defined locally; without it every icon would render as its ligature text. Verified: no googleapis/gstatic reference anywhere in the build output, `document.fonts.check()` passes for both families in a real browser with zero requests to Google, and a production build with the network blocked now succeeds. Provenance and licences (OFL 1.1 / Apache-2.0) in src/fonts/README.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The production build shipped without any Content Security Policy. The obvious remedy is also a trap: Angular's critical-CSS optimisation emits <link rel="stylesheet" ... media="print" onload="this.media='all'"> <noscript><link rel="stylesheet" ...></noscript> A CSP with script-src 'self' blocks inline event handlers, the onload never fires, the stylesheet stays on media="print" and the app renders unstyled with JavaScript enabled — the noscript copy does not help. security.autoCsp handles this: it removes the onload, replaces it with ngcspmedia plus a hashed loader script, and emits a hash-based policy (script-src 'strict-dynamic' + hashes, object-src 'none', base-uri 'self'). Verified against the built artifact served in a real browser rather than by reading the HTML: link media = all (the loader ran under the CSP) external sheets = 1, 40 rules (full stylesheet applied) body font-family = Montserrat (self-hosted font active) inline handlers = 0 CSP violations = none subresourceIntegrity adds sha384 integrity attributes to the emitted script and style tags. Limited value at same origin with outputHashing, but free. autoCsp only covers script-src, object-src and base-uri. style-src, font-src, connect-src, frame-ancestors and the transport headers are response-header concerns and remain out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a "Sicherheits-Konventionen" section capturing the rules the fixes in this branch depend on, so the findings do not come back: - an HTTP response is input, not a typed guarantee - never hand unchecked values to Transloco as parameters - never build i18n keys from data - encodeURIComponent for every value in a URL, route params included - policyId is an identifier (JSON-LD @id), keep the charset check - never discard HttpErrorResponse; never log request/response bodies - dev tooling out of the production bundle, and why the fileReplacement has to target a barrel rather than the component - no external runtime resources - production builds with autoCsp/subresourceIntegrity must be verified in a browser, not by reading index.html Also documents the new services/http and src/fonts directories and updates the E2E count to 20. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.