Per maintainer review (RohitR311 on #1138): the repo has no existing
test infra or CI to run tests, so introducing jest here is out of
scope. Removes jest configs, test files, tsconfig.test.json, the
test:server script, and jest/ts-jest/@types/jest/jest-environment-jsdom
dependencies from both maxun-core and server.
output-validation.ts stays - it's a real production module (not test
code) extracted to fix the duplicated isPartial bug CodeRabbit flagged;
only its test file is dropped.
computeCrawlSearchPersistDelta advances lastPersistedResultCount as
soon as a delta is computed, before the write is confirmed. If
flushPersistenceBuffer exhausts its retries and drops the batch, that
data was never actually written, but the cursor had already moved
past it - so it was silently lost forever instead of being resent
with the next page's delta.
Roll the cursor back by the dropped delta's size when a batch is
dropped, so the next page recovers the lost items instead of skipping
them. Covered by two new tests exercising the real retry-exhaustion
path via a rejecting sequelize.transaction mock.
If triggerIntegrationUpdates threw, the catch(_) swallowed it and left
partialDataExtracted false even though hasData was true, so isPartial
was persisted incorrectly on the failure path.
Addresses CodeRabbit review on #1138 (both findings confirmed valid
against the actual code, not just the review's claim):
- hasData's scrapeSchema/scrapeList checks used `.length > 0`, but both
are always plain objects keyed by action name, never arrays -
`.length` on a plain object is undefined, so the check could never
be true. isPartial could never reflect a run whose only partial
output was scrapeSchema/scrapeList. Pre-existing bug, not introduced
by this PR, but copied into abortRun's check when extending it here.
- abortRun reloaded the run before checking for partial data but never
forced the interpreter's persistence buffer to flush first -
scrapeList isn't on the immediate-flush allowlist, so a buffered
scrapeList page could be invisible to the reload.
Both fixes now live in one place (hasPartialOutput /
flushReloadAndCheckPartialOutput in output-validation.ts) instead of
duplicated inline in task-runner.ts's two call sites, which is how the
same bug ended up in two places to begin with.
Verified with mutation testing: reintroduced the .length > 0 bug,
confirmed 3 tests fail; reintroduced the reload-before-flush ordering
bug, confirmed the ordering test fails; restored the fix, all 14 pass.
Rebasing onto the latest develop pulled in #1105's per-page screenshot
upload path, which added a BinaryOutputService import to Interpreter.ts
that attempts a real MinIO connection at module load. Mock it so these
tests stay hermetic instead of noisily (and potentially flakily)
failing to connect to a MinIO that isn't there in CI.
Same class of bug already fixed on the maxun-core side: `tsc -p
server/tsconfig.json` (the actual build:server script) was compiling
server/src/**/__tests__ into server/dist, since the base tsconfig's
include (src/**/*) picked them up with nothing excluding them. Found
by actually running the production build rather than trusting
--noEmit type-checks, which don't reveal what gets emitted.
server had no test infra at all (no test script, no jest config). Add
jest/ts-jest scoped to server/, and directly unit test
computeCrawlSearchPersistDelta/mergeCrawlDelta/mergeSearchDelta - the
new delta-slicing and append-merge logic - without needing to mock
the database or drive a full interpreter run.
Verified these tests actually catch the regression they're meant to:
temporarily reverted the slicing to send full arrays again and
confirmed 4 of the 11 tests fail, including the end-to-end one showing
the exact page duplication the fix prevents.
crawl/search deliver the full cumulative array on every page callback,
and persistDataToDatabase was writing that whole array to Postgres on
every single page - meaning page 1's data got re-sent on every write
for the rest of the run, an O(n^2) total-bytes cost that gets worse
the more pages a crawl visits (each carrying full page HTML).
Track how many items have already been persisted per action and slice
off just the new ones before they reach the persistence buffer, then
append (rather than overwrite) that delta into the existing DB state.
The in-memory cache and socket broadcast are untouched - only what
gets written to Postgres changes. Extracted as computeCrawlSearchPersistDelta/
mergeCrawlDelta/mergeSearchDelta so this can be tested directly.
status alone doesn't distinguish a failed/aborted run that still has
usable partial data from one with nothing recovered - that signal only
existed transiently in socket/webhook payloads and was lost on reload.
Persist it as a real column, set from both the crash-catch path and
abortRun, and expose it via formatRunResponse.
Also fix abortRun's partial-data check, which only looked at
scrapeSchema/scrapeList and missed crawl/search entirely, and reload
the run before checking since the interpreter's persistence buffer
writes through a separate Run instance.
persistDataToDatabase only force-flushed the batched write immediately
for scrapeSchema; everything else, including per-page crawl/search
writes, sat in the buffer for up to 3s or 5 items before hitting
Postgres. A hard process kill inside that window would still lose
data. Extend the immediate-flush condition to crawl and search too.
Screenshots were captured per page (single scraping pass) but held in
memory as base64 for the whole run and bulk-uploaded to MinIO only at
the end. Since maxun-core awaits binaryCallback after each page scrape,
the run-mode callback is the natural upload point: page N's screenshot
now lands in MinIO and run.binaryOutput (as a public URL) before page
N+1 is scraped.
- BinaryOutputService.uploadBinaryOutputItem: single-item upload,
extracted from the bulk loop so object-key and public-URL
construction (#832) stay single-source. Returns null on failure
instead of throwing.
- Run-mode binaryCallback uploads first; on failure the item keeps its
base64 payload so the end-of-run bulk upload retries it.
- uploadAndStoreBinaryOutput passes already-uploaded URL entries
through untouched, making the end-of-run call idempotent — required
because it fully replaces run.binaryOutput.
- persistBinaryDataToDatabase stores URL entries as plain strings so
the mid-run DB shape matches the final uploaded shape.
Applies to all run-mode screenshots (crawl/search page captures and
regular screenshot actions share the same callback payload).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- validate formats in PUT /runs/:id before persisting, so a malformed
value can't reach the interpreter and break formats.includes(...)
- re-throw on failed search-result navigation so it records an error
entry instead of scraping/screenshotting the stale page (matches the
crawl loop's behavior)
- correct stale getNavigationWaitStrategy JSDoc (networkidle is driven
by visualRenderRequired, not screenshot formats)
- fix duplicated phrase in processRobotOutputFormats JSDoc
Address CodeRabbit review: the previous doc comment near
ALLOWED_NAVIGATION_PROTOCOLS overstated what the URL check protects
against. It implied that filtering by scheme alone would also block
navigation to internal HTTP(S) endpoints, which is not the case:
http://localhost, http://127.0.0.1, http://169.254.169.254, RFC1918
ranges etc. still pass because they use the http(s) scheme.
Reword the JSDoc to:
- describe this check as scheme validation only (WHATWG URL parser +
http/https allowlist)
- keep the non-network URI examples (file:, chrome:, data:, javascript:)
that this check does block
- add an explicit "Limitations" note stating that internal-host SSRF
is not mitigated and would require separate host/IP-range filtering
in the input:url handling path
No behavioural change.
The recording browser worker exposes an `input:url` socket event which
forwarded the client-supplied URL directly to `page.goto(url)` in
`handleChangeUrl`. Because Playwright runs on the server, any recording
client could coerce the shared worker into loading `file:///etc/passwd`,
`chrome://`, `about:`, `data:`, `javascript:` URIs, or arbitrary internal
HTTP endpoints. The response then becomes accessible to subsequent scrape
steps and is exfiltrated as run output.
Parse the incoming URL with the WHATWG URL API and reject anything whose
protocol is not `http:` or `https:` before generating the workflow pair
and before invoking `page.goto`. Malformed URLs (parser throws), relative
paths, protocol-relative URLs and opaque schemes are all rejected. The
rejection reuses the existing `emitBrowserPageError` broadcast so the
client sees a friendly message instead of a silent no-op.
CWE-20 (Improper Input Validation).
Screenshot/file URLs written to a run's binaryOutput were built from the
internal MinIO host/port (defaulting to localhost:9000). Any deployment
where MinIO sits behind a reverse proxy or on a non-localhost public
address produced unreachable URLs, so stored screenshots never loaded.
Prefer a complete public base URL via MINIO_PUBLIC_URL (e.g.
https://storage.example.com) and fall back to the existing
MINIO_PUBLIC_HOST:MINIO_PORT composition for local/dev setups, so existing
configurations are byte-for-byte unchanged. Documents the variable in ENVEXAMPLE.
Fixes#832
amhsirak