Skip to content

feat: redact sensitive feed data in structured logs#903

Merged
gildesmarais merged 7 commits intomainfrom
slice/log-sanitization
Mar 22, 2026
Merged

feat: redact sensitive feed data in structured logs#903
gildesmarais merged 7 commits intomainfrom
slice/log-sanitization

Conversation

@gildesmarais
Copy link
Copy Markdown
Member

@gildesmarais gildesmarais commented Mar 21, 2026

Summary

  • redact feed tokens from request-scoped logging paths
  • replace logged source URLs with hashed host metadata
  • consolidate security and observability emission through a shared structured logger
  • route rack-timeout logging through the same JSON logger

Verification

  • docker compose -f .devcontainer/docker-compose.yml up -d
  • docker exec devcontainer-app-1 bash -lc 'cd /workspace && make setup && make ready'

Notes

  • make ready passed with the new redacted log shape visible in the exercised request logs during RSpec

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 tightens security around request-scoped structured logging by redacting sensitive feed tokens and replacing logged source URLs with sanitized metadata, while consolidating observability/security emission through a shared JSON logger (including rack-timeout).

Changes:

  • Introduces AppLogger, LogEvent, and LogSanitizer to centralize structured logging and sanitize sensitive fields.
  • Updates Observability and SecurityLogger to emit through the shared structured logger.
  • Redacts /api/v1/feeds/:token in request context and routes rack-timeout logs through the same JSON formatter.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
spec/html2rss/web/request_context_middleware_spec.rb Adds coverage for redacting feed tokens in request context path.
spec/html2rss/web/log_sanitizer_spec.rb New specs for path redaction, URL sanitization, and log formatting behavior.
app/web/telemetry/observability.rb Switches observability emission to the shared LogEvent emitter.
app/web/telemetry/log_sanitizer.rb Adds sanitizers for feed-token paths and URL fields in log details.
app/web/telemetry/log_event.rb Introduces a shared emitter that merges request context + sanitized payload.
app/web/telemetry/app_logger.rb Adds a shared JSON logger/formatter (JSON + logfmt parsing).
app/web/security/security_logger.rb Routes security events through LogEvent and shared logger state.
app/web/request/request_context_middleware.rb Redacts feed tokens when building request context.
app/web/boot/setup.rb Wires rack-timeout logging to use the shared JSON logger.

Comment thread app/web/request/request_context_middleware.rb
Comment thread spec/html2rss/web/request_context_middleware_spec.rb
Comment thread app/web/security/security_logger.rb Outdated
Comment thread app/web/telemetry/app_logger.rb Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 12 out of 12 changed files in this pull request and generated 3 comments.

Comment on lines +16 to +24
def sanitize_path(path)
return if path.nil?

path_string = path.to_s
suffix = feed_suffix(path_string)
token_path = suffix ? path_string.delete_suffix(suffix) : path_string

token_path.gsub(FEED_TOKEN_ROUTE, "\\1[REDACTED]#{suffix}")
end
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

sanitize_path always strips a .json/.xml/.rss suffix before attempting the feed-token replacement. If the path ends with one of those suffixes but does not match the /api/v1/feeds/:token pattern, the method returns the suffix-stripped path, which will corrupt logged paths (e.g., /api/v1/health.json -> /api/v1/health). Consider matching the full feed-token route (including an optional suffix) and only redacting when that match succeeds, otherwise return the original path_string unchanged.

Copilot uses AI. Check for mistakes.
Comment thread spec/html2rss/web/log_sanitizer_spec.rb Outdated

RSpec.describe Html2rss::Web::LogSanitizer do
let(:io) { StringIO.new }
let(:logger) { Logger.new(io).tap { |log| log.formatter = Html2rss::Web::AppLogger.send(:method, :format_entry) } }
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

This spec sets the logger formatter via Html2rss::Web::AppLogger.send(:method, :format_entry), but format_entry is a private singleton method in AppLogger. method(:format_entry) typically raises NameError for private methods, so this can fail when running the spec. Prefer private_method(:format_entry) (or expose a small public helper on AppLogger intended for tests).

Suggested change
let(:logger) { Logger.new(io).tap { |log| log.formatter = Html2rss::Web::AppLogger.send(:method, :format_entry) } }
let(:logger) { Logger.new(io).tap { |log| log.formatter = Html2rss::Web::AppLogger.send(:private_method, :format_entry) } }

Copilot uses AI. Check for mistakes.
Comment on lines +136 to +140
# @param url [String]
# @return [Hash{Symbol=>String}]
def sanitized_url(host, url)
{ host:, scheme: 'https', hash: url_hash(url) }
end
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

The helper def sanitized_url(host, url) defined later in this spec overrides the earlier let(:sanitized_url) helper method. After this definition, any call to sanitized_url without arguments (e.g. eq(url: sanitized_url)) will raise an ArgumentError. Rename one of these helpers (e.g., expected_sanitized_url for the let, or build_sanitized_url for the helper) to avoid the method name collision.

Copilot uses AI. Check for mistakes.
@gildesmarais gildesmarais marked this pull request as ready for review March 22, 2026 12:53
@gildesmarais gildesmarais enabled auto-merge (squash) March 22, 2026 12:57
@gildesmarais gildesmarais merged commit ee7df73 into main Mar 22, 2026
12 checks passed
@gildesmarais gildesmarais deleted the slice/log-sanitization branch March 22, 2026 12:58
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