Skip to content

KNOX-3409: Validate knoxauth theme name to Harden Branding Support - #1341

Open
lmccay wants to merge 2 commits into
apache:masterfrom
lmccay:KNOX-3409
Open

KNOX-3409: Validate knoxauth theme name to Harden Branding Support#1341
lmccay wants to merge 2 commits into
apache:masterfrom
lmccay:KNOX-3409

Conversation

@lmccay

@lmccay lmccay commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

KNOX-3409 - Validate knoxauth theme name to Harden Branding Support

What changes were proposed in this pull request?

The knoxauth login page read the "theme" query parameter and wrote it into a tag via document.write() with no validation, allowing arbitrary markup injection on the page that collects user credentials. The value was also persisted to localStorage and replayed on later visits, so a single malicious link kept executing on subsequent visits from a clean URL.

Theme names are now validated against ^[a-zA-Z0-9_-]{1,64}$ on every path (URL parameter, localStorage, and the configured default), which rejects quotes, angle brackets, dots and path separators. The stylesheet element is built with DOM APIs instead of string concatenation, so a theme name can never be parsed as markup. A stored value that fails validation, or that does not resolve to an installed theme, is discarded rather than replayed.

Because the only URL the loader can produce is styles/themes//theme.css, the themes actually installed on the server remain the effective allowlist and no new configuration is required. The README and deployment guide are updated to describe the validation, replacing two security claims that were inaccurate as written.

Reported by Quinn Nguyen. Not present in any released version - the theming feature (KNOX-3283) has only ever been on master.

How was this patch tested?

Manually tested.

There is no existing test harness for this area, we will follow up with a test suite afterward.

The knoxauth login page read the "theme" query parameter and wrote it into
a <link> tag via document.write() with no validation, allowing arbitrary
markup injection on the page that collects user credentials. The value was
also persisted to localStorage and replayed on later visits, so a single
malicious link kept executing on subsequent visits from a clean URL.

Theme names are now validated against ^[a-zA-Z0-9_-]{1,64}$ on every path
(URL parameter, localStorage, and the configured default), which rejects
quotes, angle brackets, dots and path separators. The stylesheet element is
built with DOM APIs instead of string concatenation, so a theme name can
never be parsed as markup. A stored value that fails validation, or that
does not resolve to an installed theme, is discarded rather than replayed.

Because the only URL the loader can produce is styles/themes/<name>/theme.css,
the themes actually installed on the server remain the effective allowlist and
no new configuration is required. The README and deployment guide are updated
to describe the validation, replacing two security claims that were inaccurate
as written.

Reported by Quinn Nguyen. Not present in any released version - the theming
feature (KNOX-3283) has only ever been on master.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Test Results

40 tests   40 ✅  6s ⏱️
 3 suites   0 💤
 3 files     0 ❌

Results for commit e956db5.

♻️ This comment has been updated with latest results.

@lmccay
lmccay requested review from pzampino and smolnar82 and a lite review from Copilot August 8, 2026 01:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Hardens KnoxAuth login-page theming so attacker-controlled theme inputs (URL/localStorage/default) can’t inject markup or traverse paths, and updates documentation to accurately describe the security properties.

Changes:

  • Validates theme names against ^[a-zA-Z0-9_-]{1,64}$ for URL, localStorage, and configured defaults.
  • Replaces document.write() stylesheet injection with DOM-based <link> creation.
  • Updates theming documentation (security considerations and CSP guidance) to match the new behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
gateway-applications/src/main/resources/applications/knoxauth/app/styles/themes/README.md Updates security considerations to document theme-name validation and DOM-based stylesheet insertion.
gateway-applications/src/main/resources/applications/knoxauth/app/styles/themes/DEPLOYMENT.md Documents validation behavior and adds CSP deployment guidance for knoxauth.
gateway-applications/src/main/resources/applications/knoxauth/app/login.html Implements validation and safe stylesheet loading for KnoxAuth theme selection.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Addresses review feedback. The cleanup flag tracked whether the theme had
been read from localStorage rather than whether it is currently held there.
A theme supplied via ?theme= is persisted immediately, so it is equally
eligible for cleanup, but the flag stayed false and no onerror handler was
attached. A name that passed validation without resolving to an installed
theme was therefore saved and replayed once before being cleared.

The flag is renamed to themeIsPersisted and set after a successful
setItem, so it reflects the invariant the cleanup actually depends on.
Setting it inside the try means a failed write - localStorage disabled -
correctly leaves nothing to clean up. Behaviour for an admin-configured
theme is unchanged: a deployment error does not discard a user preference.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

gateway-applications/src/main/resources/applications/knoxauth/app/login.html:85

  • localStorage cleanup doesn’t remove invalid-but-falsy saved values (e.g., empty string). sanitize('') returns null, but if (saved && !theme) won’t run, so the invalid value is not discarded even though the docs/intent say failing values are discarded.
                           var saved = localStorage.getItem('knox-auth-theme');
                           theme = sanitize(saved);
                           if (saved && !theme) {
                               localStorage.removeItem('knox-auth-theme');
                           }
                           themeIsPersisted = theme !== null;

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants