From d1790b7464c4bba638bb12a361fc5f0cfaff4d6c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 16:31:48 +0000 Subject: [PATCH 1/6] P0/P1: route the backend on BTP, and close the app's security gaps ROUTING (P0) -- the deployed app could not have worked. mta.yaml has always declared an abap2UI5-srv destination pointing at the CAP module, and xs-app.json never routed anything to it: its last route is a catch-all to the static HTML5 repository, so every POST /rest/root/z2ui5 roundtrip went to the file store instead of the service. Nothing noticed, because jest runs against a local cds server that never sees the approuter and deploy-check deliberately skips mbt build. Routes added for the roundtrip, the OData service and /health; src/test/approuter-routes.test.js pins the contract, the order (backend routes must beat the catch-all), the auth types, and that every destination a route names exists in mta.yaml. csrfProtection is false on the roundtrip route deliberately: the webapp posts with no CSRF token and never fetches one, so the approuter's default would 403 every roundtrip. CSRF is defended one layer down instead -- see below. SECURITY (P1): - The services now require the declared `User` role instead of merely `authenticated-user`. xs-security.json had declared the $XSAPPNAME.User scope and a role template since the beginning and nothing referenced them, so the authorization model existed on paper while every authenticated user in the subaccount passed, role collection assigned or not. The mocked development users carry the same role, so local and test flows are unchanged. - Security headers now reach the DATA endpoints. Only the bootstrap page applied the framework's t_security_header; the roundtrip and the OData entities -- the responses that actually carry application state -- answered with no nosniff, no frame guard, and cacheable by any intermediary. - The request body is capped explicitly (Z2UI5_MAX_BODY, default 2mb). The action takes an @open object, so CDS validates nothing, and the only limit was express's 100kb default applying by accident rather than by decision. - Draft retention had disagreed with the framework about its own TTL -- this job deleted at 24h while the exit's draft_exp_time_in_hours said 4h, with nothing connecting them. The framework value is now the source of truth and the env var overrides both. It also ran on every instance, so N instances meant N concurrent hourly DELETEs over the same rows; it now runs on one (Z2UI5_DRAFT_RETENTION_INSTANCE, default CF instance 0). - Indexes for the two non-primary-key access paths, as HANA design-time artifacts in db/src: createdAt, because the hourly retention DELETE was a full scan whose cost grew with the very table it exists to contain, and owner, because the service projection filters on it. Verified to reach gen/db/src through `cds build --production`. - mta.yaml declares instances: 1. Sticky app state lives in the serving process with no session affinity configured, so scaling out produces intermittent, hard-to-reproduce state loss. The comment says what to fix before raising it. - dependabot gains the npm ecosystem in the app repo, which it never had -- no runtime dependency of the deployed app was ever proposed for update. CAP packages are grouped (they version together); openui5-dist is ignored, because that pin is a compatibility decision tracked upstream. - npm audit runs on the shipped lock in the app repo's CI, advisory for now. README's security section documented the role and CSRF gaps honestly; it now documents what replaced them, and the sticky/multi-instance limitation that remains. builder 30 tests, app 44 tests (was 23), lint clean, cds build --production ok. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01T2gZuy95uKcT2zngQd1sAL --- run/input/UPSTREAM_COMMIT | 2 +- run/input/core/package.json | 1 + run/input/core/srv/z2ui5/00/03/z2ui5_html.js | 73 +++++++++++ .../srv/z2ui5/01/02/z2ui5_cl_ui5_app_cont.js | 9 ++ .../z2ui5/01/03/z2ui5_cl_ui5f_index_html.js | 31 ++++- .../srv/z2ui5/01/04/z2ui5_cl_ui5_user_exit.js | 21 +++ .../srv/z2ui5/02/z2ui5_cl_ui5_http_handler.js | 53 ++++++-- run/input/core/srv/z2ui5/02/z2ui5_if_types.js | 1 + run/input/core/srv/z2ui5/z2ui5_port.js | 60 ++++++++- src/README.md | 41 +++++- src/app/z2ui5/xs-app.json | 19 +++ src/db/schema.cds | 6 + src/db/src/z2ui5_t_01_createdat.hdbindex | 9 ++ src/db/src/z2ui5_t_01_owner.hdbindex | 8 ++ src/mta.yaml | 12 ++ src/package.json | 6 +- src/srv/draft-retention.js | 63 ++++++++- src/srv/server.js | 30 +++++ src/srv/z2ui5-service.cds | 27 ++-- src/test/approuter-routes.test.js | 100 +++++++++++++++ src/test/security.test.js | 121 ++++++++++++++++++ 21 files changed, 653 insertions(+), 40 deletions(-) create mode 100644 run/input/core/srv/z2ui5/00/03/z2ui5_html.js create mode 100644 src/db/src/z2ui5_t_01_createdat.hdbindex create mode 100644 src/db/src/z2ui5_t_01_owner.hdbindex create mode 100644 src/test/approuter-routes.test.js create mode 100644 src/test/security.test.js diff --git a/run/input/UPSTREAM_COMMIT b/run/input/UPSTREAM_COMMIT index b29afff..fa85b1e 100644 --- a/run/input/UPSTREAM_COMMIT +++ b/run/input/UPSTREAM_COMMIT @@ -1 +1 @@ -7d1d58fb03e25330c632466412fd3824a7ee9ca8 +21bf2a69ea941e0c522eaf85c2495e0eff5bc87d diff --git a/run/input/core/package.json b/run/input/core/package.json index ec820a6..d6b2cff 100644 --- a/run/input/core/package.json +++ b/run/input/core/package.json @@ -19,6 +19,7 @@ "./cx_root": "./srv/z2ui5/00/00/cx_root.js", "./z2ui5_cl_ui5_util_*": "./srv/z2ui5/00/03/z2ui5_cl_ui5_util_*.js", "./z2ui5_cx_ui5_util_error": "./srv/z2ui5/00/03/z2ui5_cx_ui5_util_error.js", + "./z2ui5_html": "./srv/z2ui5/00/03/z2ui5_html.js", "./z2ui5_cl_ui5_srv_draft": "./srv/z2ui5/01/01/z2ui5_cl_ui5_srv_draft.js", "./z2ui5_cl_ui5_app_cont": "./srv/z2ui5/01/02/z2ui5_cl_ui5_app_cont.js", "./z2ui5_cl_ui5_app_*": "./srv/z2ui5/01/04/z2ui5_cl_ui5_app_*.js", diff --git a/run/input/core/srv/z2ui5/00/03/z2ui5_html.js b/run/input/core/srv/z2ui5/00/03/z2ui5_html.js new file mode 100644 index 0000000..b4f32f6 --- /dev/null +++ b/run/input/core/srv/z2ui5/00/03/z2ui5_html.js @@ -0,0 +1,73 @@ +/** + * z2ui5_html — HTML escaping for the bootstrap page. + * + * WHY THIS EXISTS + * --------------- + * The bootstrap page (z2ui5_cl_ui5f_index_html, and the equivalent builder in + * z2ui5_cl_ui5_http_handler._http_get) interpolates exit-supplied configuration + * straight into markup: the tab title into , the favicon URI into a + * <link href>, the bootstrap src and theme into script attributes, and every + * t_add_config row into a single-quoted data-sap-ui-* attribute. + * + * None of it was escaped. That was survivable only as long as every value is a + * constant the framework itself sets — but the exit receives the request + * context (init_context / set_config_http_get see path, params and headers), so + * the moment an app reflects a query parameter into its title or into an extra + * config row, the page hands an attacker an injection point. A `'` closes the + * attribute; a `` closes the element. + * + * These helpers make that impossible by construction, and give an exit author + * something to call for their own interpolation: + * + * const { escape_text, escape_attr } = require("abap2UI5/z2ui5_html"); + * + * Escaping is deliberately conservative — the same replacements in both + * helpers plus the quote characters — so a value is safe in element text, in a + * single-quoted attribute and in a double-quoted one alike, and a caller + * cannot pick the wrong one. + */ +"use strict"; + +const REPLACEMENTS = [ + [/&/g, `&`], // first, or it would double-escape the entities below + [//g, `>`], + [/"/g, `"`], + [/'/g, `'`], +]; + +function escape(val) { + let out = val === null || val === undefined ? `` : String(val); + for (const [re, to] of REPLACEMENTS) out = out.replace(re, to); + return out; +} + +/** Escape a value for HTML element text (e.g. between and ). */ +module.exports.escape_text = escape; + +/** Escape a value for an HTML attribute, single- or double-quoted. */ +module.exports.escape_attr = escape; + +/** + * Escape a URI destined for a DOUBLE-QUOTED attribute (href="…", src="…"), + * and reject the schemes a URI context makes dangerous: `javascript:` and + * `vbscript:` execute on click or on load. A rejected value yields the empty + * string, which callers treat as "not configured" and omit the element + * entirely rather than emitting a live but broken attribute. + * + * Escapes only `&` and `"` — the two characters that can end the attribute or + * start an entity in this context. It deliberately does NOT escape `<`, `>` + * or `'`: they are inert inside a double-quoted attribute value, and encoding + * them would corrupt legitimate URIs. The shipped favicon is the worked + * example — `data:image/svg+xml,` has to survive intact, + * and upstream's own test asserts the page contains `data:image/svg+xml, tag by contract, not a value. + const html = require(`../../00/03/z2ui5_html`); const csp = cfg.content_security_policy || ``; - const title = cfg.title || `cap2UI5`; - const theme = cfg.theme || `sap_horizon`; - const src = cfg.src || `https://sdk.openui5.org/resources/sap-ui-cachebuster/sap-ui-core.js`; + const title = html.escape_text(cfg.title || `cap2UI5`); + const theme = html.escape_attr(cfg.theme || `sap_horizon`); + const src = html.escape_uri(cfg.src || `https://sdk.openui5.org/resources/sap-ui-cachebuster/sap-ui-core.js`); - // Extra \t \t${title} - +${favicon} \t\n` + - `