Skip to content

[security] Architectural: introduce central output sink + LOGGER + typed error class to jira/gitlab skills #1555

Description

Background

The Mural skill's recent OAuth security hardening landed three coupled
abstractions that together form the redaction contract:

  • LOGGER (a module-level logging.getLogger(name)) — single observable
    log surface.
  • _emit() / _emit_debug_traceback() — single output sink that runs
    every formatted string through _redact() before reaching LOGGER or
    stderr.
  • MuralAPIError — typed error class with a controlled __str__ (status,
    code, message, request_id) that never carries raw header dicts or response
    bodies into exception chains.

References:

  • .github/skills/mural/mural/scripts/mural.py L298 (LOGGER), L312
    (MuralAPIError), L1540 (_emit), L1547 (_emit_debug_traceback)
  • .github/skills/mural/mural/SECURITY.md Bucket B2 / B4 Information
    Disclosure rows

Without these three abstractions, redaction is unenforceable — a
contributor can always reach print(..., file=sys.stderr) directly, and a
freshly-raised exception will carry the raw upstream body into the traceback.
This is precisely the structural pattern that allowed G-INF-1
(client_secret not in the keyset) to remain undetected for as long as it
did, until the central sink and source-contract tests were introduced.

Audit findings

.github/skills/jira/jira/scripts/jira.py (472 LOC)

Site Gap
L37 class ScriptError(Exception) Generic; no status/code/request_id; no controlled __str__
L97 def request(...) No LOGGER, no debug logging, no central sink
L120, L124-128, L132-134 Raw URL + raw upstream body embedded in exception text
L143-159 _extract_error_message Returns upstream body verbatim
L204 _print_selected_fields, L218 _print_result Raw print() of upstream JSON
L323 handle_comment reads sys.stdin.read() bare Raw payload could land in error traceback

.github/skills/gitlab/gitlab/scripts/gitlab.py (439 LOC)

Site Gap
L34 gitlab_url, gitlab_token, api_url module globals Mutable global secret state
L40 die() helper Raw print(..., file=sys.stderr) + SystemExit; no error class at all
L122 def request(...) No LOGGER, no central sink
L154-159, L161, L168, L172 Raw upstream body / URL into stderr or stdout
L387-394 cmd_job_log second urlopen site Bypasses request() entirely; raw print(error.read().decode(), file=sys.stderr)
L218 print_fields() Raw print() of arbitrary upstream field values
L427-437 main() Bare print(..., file=sys.stderr) for KeyboardInterrupt + BrokenPipeError

Why this is an architectural prerequisite, not a refactor

Adding _REDACT_KEYS / _REDACT_PATTERNS / _redact() to either skill
without first introducing LOGGER + _emit() + a typed error class
results in a redaction net with holes wherever a print() or a raw exception
chain still exists. Specifically:

  1. The redaction call must live somewhere — _emit() is that somewhere.
    Otherwise, every site that wants to log must remember to call _redact(),
    which is exactly the failure mode source-contract tests exist to prevent.
  2. Raised exceptions are formatted by Python at the top of the stack — if the
    error message contains an unfiltered upstream body, the traceback printed
    on --no-debug paths leaks it. A typed error class with a controlled
    __str__ is the choke point.
  3. GitLab's cmd_job_log second urlopen site (L387-394) is the cleanest
    evidence: even after redaction is added to request(), this site would
    continue to leak. It must be refactored to route through request() (or
    through a shared _http_get_raw() helper that itself uses _emit() for
    error paths).

This issue must merge before the Jira and GitLab redaction-port issues
land.

Required changes

Common to both skills

  1. LOGGER = logging.getLogger("jira") / logging.getLogger("gitlab").
  2. _emit(level, message, *args) — formats, redacts, routes to LOGGER.log
    and (for user-facing errors) to sys.stderr.
  3. _emit_debug_traceback(exc) — gated on JIRA_DEBUG / GITLAB_DEBUG;
    calls traceback.format_exception and pipes the result through
    _redact() (depends on the redaction-port issues for the actual
    _redact function, but the call site lands here).
  4. Typed error class with controlled __str__:
    • JiraAPIError(ScriptError) — fields: status, code, message,
      request_id. Replaces raw ScriptError(f"HTTP ... details ...") at
      L124-128 / L132-134.
    • GitLabAPIError(Exception) — same field set. Replaces every die()
      call site (gitlab has no error class at all today).
  5. Refactor every existing print(...) / print(..., file=sys.stderr) /
    die(...) to route through _emit() or raise <error class>(...).

GitLab-specific

  1. Refactor cmd_job_log L387-394: collapse the second urlopen site
    into the central request() helper. Add a source-contract test asserting
    exactly one urlopen call site in the file.
  2. Refactor main() L427-437 (KeyboardInterrupt, BrokenPipeError) to
    route through _emit().
  3. Move gitlab_token off the module global into a small immutable config
    object mirroring Jira's JiraClient dataclass.

Acceptance criteria

  • Both skills define LOGGER, _emit, _emit_debug_traceback, and a
    typed API error class.
  • Source-contract test: zero print(..., file=sys.stderr) outside
    _emit().
  • Source-contract test: zero LOGGER.exception( calls (mirror
    .github/skills/mural/mural/tests/test_redaction.py
    test_logger_no_bare_exception_calls).
  • GitLab: source-contract test asserts exactly one urlopen call site.
  • GitLab: no die() helper remaining; all error paths use the
    GitLabError hierarchy, and API transport failures use
    GitLabAPIError.
  • GitLab: gitlab_token is no longer a module-level mutable global.
  • Jira: JiraClient.__repr__ does not contain the raw auth_header
    value.
  • Both skills: typed error class __str__ excludes raw upstream body
    (negative test).

Dependencies

  • Blocks the Jira redaction-port issue.
  • Blocks the GitLab redaction-port issue.
  • Blocks the per-skill CI gating issue.

References

  • .github/skills/mural/mural/scripts/mural.py L298, L312, L1540, L1547
  • .github/skills/mural/mural/tests/test_redaction.py source-contract section
  • docs/security/security-model.md OA-1..OA-17

Metadata

Metadata

Labels

priority-1Critical priority, address immediatelysecuritySecurity-related changes or concernsskillsCopilot skill packages (SKILL.md)tech-debt

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions