fix: sanitize untrusted attributes and preserve fingerprints - #423
fix: sanitize untrusted attributes and preserve fingerprints#423stroland02 wants to merge 1 commit into
Conversation
![superlog-app[bot]](https://avatars.githubusercontent.com/in/3485414?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (stampedCount === 0) return { body, stampedCount }; | ||
| if (stampedCount === 0 && sanitizedCount === 0) return { body, stampedCount }; |
There was a problem hiding this comment.
logs · warning — Log when untrusted superlog. attributes are sanitized*
Emit an info log (with sanitizedCount and the request's resource service.name) whenever sanitizedCount > 0 so operators can detect abusive or misconfigured clients attempting to inject fingerprints. Without it, attribute stripping is completely silent and impossible to alert on or audit.
| if (stampedCount === 0 && sanitizedCount === 0) return { body, stampedCount }; | |
| if (stampedCount === 0 && sanitizedCount === 0) return { body, stampedCount }; | |
| if (sanitizedCount > 0) { | |
| logger.info({ sanitizedCount, signal: "traces" }, "sanitized untrusted superlog.* attributes from inbound payload"); | |
| } | |
| return { body: Buffer.from(JSON.stringify(payload)), stampedCount }; |
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| if (!hasSuperlog) return attrs; | ||
| return attrs.filter((attr) => !(attr.key || "").startsWith("superlog.")); |
There was a problem hiding this comment.
metrics · warning — Add a counter for sanitized untrusted attribute removals
Increment a module-scope counter (e.g. superlog.proxy.sanitized_attributes_total with dimension signal: traces|logs) each time sanitizeSuperlog removes at least one key, so operators can chart injection attempt rates and alert on spikes without parsing logs.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/proxy/src/ingest-fingerprints.ts">
<violation number="1" location="apps/proxy/src/ingest-fingerprints.ts:287">
P1: Compressed or oversized payloads bypass this sanitizer and can retain a sender-forged `superlog.issue_fingerprint` on the direct ClickHouse path. Apply sanitization after bounded decompression, or keep this key stripped when a payload was not sanitized.</violation>
<violation number="2" location="apps/proxy/src/ingest-fingerprints.ts:287">
P1: Protobuf OTLP log requests still bypass sanitization, so a sender can supply `superlog.issue_fingerprint` and have it retained by the direct ClickHouse mapper. Sanitize protobuf `/v1/logs` records before decode/mapping or add a protobuf log stamping/sanitizing path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| return next; | ||
| } | ||
|
|
||
| function sanitizeSuperlog(attrs: OtlpKeyValue[]): OtlpKeyValue[] { |
There was a problem hiding this comment.
P1: Compressed or oversized payloads bypass this sanitizer and can retain a sender-forged superlog.issue_fingerprint on the direct ClickHouse path. Apply sanitization after bounded decompression, or keep this key stripped when a payload was not sanitized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/proxy/src/ingest-fingerprints.ts, line 287:
<comment>Compressed or oversized payloads bypass this sanitizer and can retain a sender-forged `superlog.issue_fingerprint` on the direct ClickHouse path. Apply sanitization after bounded decompression, or keep this key stripped when a payload was not sanitized.</comment>
<file context>
@@ -237,6 +284,18 @@ function setStringAttribute(attrs: OtlpKeyValue[], key: string, value: string):
return next;
}
+function sanitizeSuperlog(attrs: OtlpKeyValue[]): OtlpKeyValue[] {
+ let hasSuperlog = false;
+ for (let i = 0; i < attrs.length; i++) {
</file context>
| return next; | ||
| } | ||
|
|
||
| function sanitizeSuperlog(attrs: OtlpKeyValue[]): OtlpKeyValue[] { |
There was a problem hiding this comment.
P1: Protobuf OTLP log requests still bypass sanitization, so a sender can supply superlog.issue_fingerprint and have it retained by the direct ClickHouse mapper. Sanitize protobuf /v1/logs records before decode/mapping or add a protobuf log stamping/sanitizing path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/proxy/src/ingest-fingerprints.ts, line 287:
<comment>Protobuf OTLP log requests still bypass sanitization, so a sender can supply `superlog.issue_fingerprint` and have it retained by the direct ClickHouse mapper. Sanitize protobuf `/v1/logs` records before decode/mapping or add a protobuf log stamping/sanitizing path.</comment>
<file context>
@@ -237,6 +284,18 @@ function setStringAttribute(attrs: OtlpKeyValue[], key: string, value: string):
return next;
}
+function sanitizeSuperlog(attrs: OtlpKeyValue[]): OtlpKeyValue[] {
+ let hasSuperlog = false;
+ for (let i = 0; i < attrs.length; i++) {
</file context>
1 participant
Resolves #285.
What was causing the issue?
The Solution
Summary by cubic
Sanitizes untrusted
superlog.*attributes before enrichment and preservessuperlog.issue_fingerprintthrough both the direct ClickHouse path and the collector. Fixes #285 so fingerprints persist while othersuperlog.*keys are stripped.sanitizeSuperloginingest-fingerprints.tsto drop untrustedsuperlog.*attrs on traces and logs before stamping fingerprints.stripSuperloginotlp-clickhouse.tsto keepsuperlog.issue_fingerprint; tests adjusted to reflect preservation.superlog.issue_fingerprintaround key stripping and remove the temp key.Written for commit aab9e20. Summary will update on new commits.