Skip to content

GUACAMOLE-2302: Add test coverage for TokenFilter parameter token grammar#1231

Open
ciroiriarte wants to merge 1 commit into
apache:mainfrom
ciroiriarte:feature/tokenfilter-grammar-tests
Open

GUACAMOLE-2302: Add test coverage for TokenFilter parameter token grammar#1231
ciroiriarte wants to merge 1 commit into
apache:mainfrom
ciroiriarte:feature/tokenfilter-grammar-tests

Conversation

@ciroiriarte

Copy link
Copy Markdown

Adds characterization tests for the parameter token grammar accepted by TokenFilter.

TokenFilterTest covers basic substitution, escaping, and the LOWER/UPPER modifiers. What isn't covered is the boundary of the grammar itself:

  • which candidate token names the pattern accepts (currently [A-Za-z0-9_]*)
  • how an unrecognized modifier is handled (currently falls through to default: and appends the value unchanged)
  • how strict mode treats text that resembles a token but is not one (currently not matched at all, so passed through literally rather than reported undefined)

That last point is load-bearing. filterStrict() has a single caller in the tree — VaultUserContext.getTokens() — which resolves secret names from vault-token-mapping.yml and treats GuacamoleTokenUndefinedException as a signal to skip that secret:

catch (GuacamoleTokenUndefinedException e) {
    logger.debug("Secret for token \"{}\" will not be retrieved. ...");
    continue;
}

So a change in which strings the token pattern matches can change whether a vault secret is retrieved at all, with only a debug-level log. These tests pin down the current behavior so that any future change to the pattern is evaluated against it rather than discovered later.

No production code is changed — this is test coverage only.

The tests also record two existing behaviors that are easy to alter unintentionally:

  • $${TOKEN} escaping, including in combination with modifiers
  • directly adjacent tokens (${A}${B}) substitute only the first, because the pattern matches one character ahead of each token to detect escaping and that character is consumed by the preceding match

For context, this came out of reviewing #1216, which proposes widening the token pattern. The tests are independent of that PR and apply to main as it stands.

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.

1 participant