feat: experimental traffic analysis#2848
Conversation
🦋 Changeset detectedLatest commit: c424e5a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
sobanieca-redocly
changed the title
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Performance Benchmark (Lower is Faster)
|
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
|
@cursor review |
|
@cursor review |
|
@cursor review |
|
@cursor review |
| 'packages/**/__tests__/**/*', | ||
| 'packages/cli/src/index.ts', | ||
| 'packages/cli/src/utils/assert-node-version.ts', | ||
| 'packages/cli/src/commands/drift/**', |
There was a problem hiding this comment.
Is it fine if we ignore coverage for experimental commands? I would expect that source code there may change significantly over time after getting feedback on usage....
| maxFindings?: number; | ||
| } | ||
|
|
||
| const ANSI = { |
There was a problem hiding this comment.
we have colorette as a dep in this repo, let's use it
| import { handleDrift, type DriftArgv } from './commands/drift/index.js'; | ||
| import type { FindingSeverity, MatchMode, TrafficFormat } from './commands/drift/types/index.js'; |
| yargs | ||
| .env('REDOCLY_CLI_DRIFT') | ||
| .positional('traffic', { | ||
| describe: 'Path to a traffic log file or folder (HAR, Kong, Nginx/Apache JSON, NDJSON).', |
There was a problem hiding this comment.
does it support those other formats?
There was a problem hiding this comment.
Yes, it should support other formats.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0e8c763. Configure here.
Co-authored-by: Jacek Łękawa <164185257+JLekawa@users.noreply.github.com>
| type Options, | ||
| type ValidateFunction, | ||
| type Ajv2020 as Ajv2020Instance, | ||
| } from 'ajv/dist/2020.js'; |
There was a problem hiding this comment.
wouldn't it be better to use our fork @redocly/ajv, this could unify usage in the repo
| method: params.method, | ||
| url: params.forwardUrl.toString(), | ||
| httpVersion: `HTTP/${params.req.httpVersion}`, | ||
| cookies: parseCookieHeader(singleHeader(params.req.headers.cookie)), |
There was a problem hiding this comment.
maybe it's worth cleaning up sensitive data ?
There was a problem hiding this comment.
For now this command should be used for short-lived local capture (like during e2e tests etc). Not sure about use cases yet, I would wait until we get more feedback from users before designing proper sensitive data removal...
| findings.push(...ruleFindings); | ||
| } | ||
| } catch (error) { | ||
| findings.push({ |
There was a problem hiding this comment.
is it actually finding? maybe simply notify the user via the logger?
There was a problem hiding this comment.
Used logger instead. Thanks!
| * JSON object) and serialized through a promise chain so concurrent captures | ||
| * never interleave file writes. | ||
| */ | ||
| export class HarWriter { |
There was a problem hiding this comment.
keeps all entries in memory and re-serializes + rewrites the entire file on every request
Maybe it would be possible to add debouncing or stream the writes using something like stream-json:
https://www.npmjs.com/package/stream-json
There was a problem hiding this comment.
I've changed the idea here. We will store data in temporary file which get's converted into HAR. This should cut memory usage.
| return '***'; | ||
| } | ||
|
|
||
| return `${value.slice(0, 4)}…`; |
There was a problem hiding this comment.
Removed magic number and named it MASK_REVEALED_PREFIX_LENGTH. This is just arbitrary number on how many characters we want to show from masked value.
| reason: string; | ||
| } | ||
|
|
||
| interface SkippedSecurityCheck { |
There was a problem hiding this comment.
Duplicate removed. Thanks.
| return { specFiles: [specPath], fromDirectory: false }; | ||
| } | ||
|
|
||
| const YAML_OPENAPI_ROOT_KEY_RE = /^(['"]?)openapi\1\s*:/m; |
There was a problem hiding this comment.
is this changes necessary ?
There was a problem hiding this comment.
Yes - this is to avoid typescript issues between drift --format and lint --format
There was a problem hiding this comment.
But I'm checking for some alternatives. Maybe indeed we can avoid changes in lint 👀
There was a problem hiding this comment.
Renamed param to report-format with alias. This allows to get rid of overlap with lint command types.
There was a problem hiding this comment.
Is the plugin system compatible with the one that's already in the core? I think having two different plugin systems for the cli is a bad idea
There was a problem hiding this comment.
In this case we are dealing with different purpose of rules (in this case traffic data). This means different data contracts etc. I would defer this unification when this command goes out of experimental phase.
There was a problem hiding this comment.
can you add documentation for v2 for both commands?
There was a problem hiding this comment.
Added docs, but @JLekawa I will need your re-review on this one. Thanks!
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c424e5a. Configure here.
| )}), or use --server to declare the server the traffic was captured against.`; | ||
| logger.warn( | ||
| `None of the ${validatedExchanges} validated exchange(s) matched a documented operation. ${hint}\n` | ||
| ); |
There was a problem hiding this comment.
Misleading server mismatch warning
Medium Severity
After a run where every validated exchange fails operation matching, warnWhenNothingMatched always logs a hint to fix host, base path, or --server, even when traffic URLs already use the same host as the description servers. That message appears alongside correct undocumented-endpoint findings and points users at the wrong cause.
Reviewed by Cursor Bugbot for commit c424e5a. Configure here.
4 participants
What/Why/How?
Added two new (experimental) commands -
driftandproxyfor collecting traffic data and validating it against provided OpenAPI definition.Reference
Testing
Screenshots (optional)
Check yourself
Security
Note
Medium Risk
Large new experimental surface (proxy forwarding, auth/security heuristics on captured traffic) but isolated commands with no changes to existing CLI auth flows; flags and behavior may still evolve.
Overview
Adds two experimental CLI commands for API contract validation against real HTTP traffic, shipped as a minor
@redocly/clirelease with docs and changesets.driftingests recorded traffic (HAR, Kong, Nginx/Apache JSON, NDJSON, plus pluggable parsers), indexes OpenAPI 3.x via@redocly/openapi-core, matches exchanges to operations (--match-modeor--server), and runs builtin/custom rules. Reports exit 1 on error-level findings and support pretty, json, csv, and sarif output.proxyruns a local reverse proxy (undiciupstream) that streams HAR entries to disk and optionally reuses the sameValidationSessionasdriftfor live findings plus a shutdown report.Shared engine includes AJV schema validation (readOnly/writeOnly handling), rules for undocumented routes, schema consistency, security baselines, and opt-in OWASP API Top 10 heuristics. E2E snapshots cover formats and report outputs.
Reviewed by Cursor Bugbot for commit c424e5a. Bugbot is set up for automated code reviews on this repo. Configure here.