Skip to content

fix: don't create log file by default, never log full environment#386

Merged
dividedmind merged 2 commits into
masterfrom
claude/appmap-agent-logging-security-ja6wkx
Jul 10, 2026
Merged

fix: don't create log file by default, never log full environment#386
dividedmind merged 2 commits into
masterfrom
claude/appmap-agent-logging-security-ja6wkx

Conversation

@dividedmind

@dividedmind dividedmind commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

A user hit a real security incident: the agent silently wrote appmap.log (including a full dump of os.environ, i.e. any secrets in the process environment) to disk, and the file got accidentally committed to git.

Two bugs combined to cause this:

  • appmap-python (the wrapper script) had a bug where APPMAP_DISABLE_LOG_FILE was always set to "false", regardless of the --enable-log/--no-enable-log flag — it checked a namespace key (no_enable_log) that never actually exists, so the log file was always created even though --help documents the flag's default as False.
  • _appmap/env.py independently defaulted to creating the log file (APPMAP_DISABLE_LOG_FILE defaulting to "false") when the wrapper wasn't used at all.
  • _appmap/configuration.py logged the entire os.environ at startup (logger.info("env: %r", os.environ)), which is how arbitrary secrets ended up in the file once one existed.

While fixing this, also found and fixed a separate wrapper-only bug: appmap-python is itself instrumented at interpreter startup (via appmap.pth — AppMap instruments any Python process by default), before runner.py's own code computes the environment the target command should run with. That incidental self-init can set internal, process-scoped _APPMAP* state using whatever it inherited — notably the once-per-process guard around the startup config-dump log lines — which then leaked into the exec'd child via os.execvpe's environment inheritance, silently suppressing that child's own startup logging even when everything else was configured correctly (e.g. appmap-python --enable-log flask run).

Changes

  • appmap/command/runner.py: fixed --enable-log/--no-enable-log to actually control APPMAP_DISABLE_LOG_FILE; strips all _APPMAP*-prefixed env vars before exec'ing the child so none of the wrapper's own incidental self-init state can leak through.
  • _appmap/env.py: APPMAP_DISABLE_LOG_FILE now defaults to true — no log file unless a user explicitly opts in.
  • _appmap/configuration.py: only the exact APPMAP/_APPMAP settings and APPMAP_*/_APPMAP_*-prefixed settings are logged at startup, never the full environment or unrelated variables that merely share the prefix (e.g. APPMAPX_*).
  • ci/tests/smoketest.sh: updated the read-only-log-file smoketest to explicitly opt in to log file creation, since it's no longer on by default.
  • _appmap/test/test_runner.py: coverage for the corrected --enable-log behavior and for the _APPMAP* leak fix.

Test plan

  • appmap-python pytest _appmap/test/test_runner.py _appmap/test/test_env.py _appmap/test/test_configuration.py — all pass, including new tests
  • Manually verified: running via appmap-python python script.py with no flags creates no appmap.log
  • Manually verified: APPMAP_DISABLE_LOG_FILE=false opts back in, and a planted secret env var no longer appears in the resulting log (only APPMAP_* settings do)
  • Manually verified with a real Flask app: appmap-python --enable-log flask run now correctly logs the startup config dump, which it silently dropped before the _APPMAP*-leak fix

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

This PR addresses a security incident by making log file creation opt-in by default and preventing accidental leakage of sensitive environment variables into appmap.log.

Changes:

  • Fixes appmap-python CLI flag handling so --enable-log/--no-enable-log correctly controls APPMAP_DISABLE_LOG_FILE.
  • Changes the agent default to not create a log file unless explicitly enabled.
  • Stops logging the full process environment at startup; logs only AppMap-related environment settings instead.
  • Updates CI smoketest and adds runner test coverage for the corrected logging behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ci/tests/smoketest.sh Forces log-file creation opt-in for the “log file not writable” smoketest, aligned with new default behavior.
appmap/command/runner.py Corrects APPMAP_DISABLE_LOG_FILE computation based on --enable-log/--no-enable-log.
_appmap/test/test_runner.py Adds test coverage for default/flag-driven log-file disabling behavior.
_appmap/env.py Changes default to disable log file unless user opts in.
_appmap/configuration.py Prevents logging full os.environ; logs only AppMap-related env settings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread appmap/command/runner.py
Comment thread _appmap/configuration.py Outdated
dividedmind pushed a commit that referenced this pull request Jul 8, 2026
Addresses Copilot review feedback on #386: startswith(("APPMAP",
"_APPMAP")) also matched unrelated variables like APPMAPX_FOO. Match
the exact APPMAP/_APPMAP names plus the APPMAP_/_APPMAP_ prefixes
instead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Nw1eEFHvABs5hdjMej5bf
claude added 2 commits July 9, 2026 20:11
A user hit a real security incident: the agent silently wrote appmap.log
(including a full dump of os.environ, i.e. any secrets in the process
environment) to disk, and the file got accidentally committed to git.

Two bugs combined to cause this:
- appmap-python (the wrapper script) had a bug where APPMAP_DISABLE_LOG_FILE
  was always set to "false", regardless of the --enable-log/--no-enable-log
  flag — it checked a namespace key (no_enable_log) that never actually
  exists, so the log file was always created even though --help documents
  the flag's default as False.
- _appmap/env.py independently defaulted to creating the log file
  (APPMAP_DISABLE_LOG_FILE defaulting to "false") when the wrapper wasn't
  used at all.
- _appmap/configuration.py logged the entire os.environ at startup, which is
  how arbitrary secrets ended up in the file once one existed.

Changes:
- appmap/command/runner.py: fixed --enable-log/--no-enable-log to actually
  control APPMAP_DISABLE_LOG_FILE.
- _appmap/env.py: APPMAP_DISABLE_LOG_FILE now defaults to true — no log file
  unless a user explicitly opts in.
- _appmap/configuration.py: only the exact APPMAP/_APPMAP settings and
  APPMAP_*/_APPMAP_* prefixed settings are logged at startup, never the full
  environment or unrelated variables that merely share the prefix (e.g.
  APPMAPX_*).
- ci/tests/smoketest.sh: updated the read-only-log-file smoketest to
  explicitly opt in to log file creation, since it's no longer on by
  default.
- _appmap/test/test_runner.py: added coverage locking in the corrected
  --enable-log behavior.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Nw1eEFHvABs5hdjMej5bf
appmap-python is a plain Python script, so it gets self-instrumented at
interpreter startup via appmap.pth (AppMap instruments any process by
default), before runner.py's own code has computed the environment the
target command should actually run with. That incidental self-init
writes internal, process-scoped state under _APPMAP*-prefixed env var
names using whatever it inherited — notably _APPMAP_MESSAGES_SHOWN, the
once-per-process guard around the startup config-dump log lines.

Since os.execvpe inherits the current environment, that stale marker
carried into the exec'd child, so anyone using the wrapper (e.g.
`appmap-python --enable-log flask run`) never saw the config-dump log
lines, even with everything else configured correctly: the wrapper's
own throwaway initialization had already tripped the "already shown"
guard before the child process's real initialization ran.

Strip all _APPMAP*-prefixed env vars before exec'ing the child and let
the already-computed envvars re-set whatever it actually needs, so the
child always starts from a clean slate regardless of what the wrapper's
incidental self-init happened to do.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Nw1eEFHvABs5hdjMej5bf
@dividedmind dividedmind force-pushed the claude/appmap-agent-logging-security-ja6wkx branch from 72c796e to cc7ea5a Compare July 9, 2026 20:11
@dividedmind dividedmind requested a review from Copilot July 9, 2026 20:39

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@dividedmind dividedmind merged commit 6594576 into master Jul 10, 2026
7 of 9 checks passed
@dividedmind dividedmind deleted the claude/appmap-agent-logging-security-ja6wkx branch July 10, 2026 10:49
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.

3 participants