GeezerVison

Copy link

Copy Markdown

Contributor

hari-kuriakose requested review from chandrasekharan-zipstack, jaseemjaskp and ritwik-g July 16, 2024 06:17

chandrasekharan-zipstack approved these changes Jul 16, 2024

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jaseemjaskp approved these changes Jul 16, 2024

View reviewed changes

muhammad-ali-e reviewed Jul 16, 2024

View reviewed changes

johnyrahul requested a review from muhammad-ali-e July 16, 2024 10:50

johnyrahul deleted the fix/rel-tag-jump branch July 16, 2024 10:50

* * Fix release tag for version update via run platform script
* Rename version update command
* Improve readability of script output
* Update docstring

* Fix tags fetch to be explicit

* Update docker/scripts/merge_env.py

Co-authored-by: ali <[email protected]>
Signed-off-by: Rahul Johny <[email protected]>

---------

Signed-off-by: Rahul Johny <[email protected]>
Co-authored-by: Rahul Johny <[email protected]>
Co-authored-by: ali <[email protected]>

CodeQL flagged str(RedirectRefused) reaching the response
(py/stack-trace-exposure, #487/#488). The caller now gets a fixed
message chosen by the exception's subclass; the detailed reason stays in
the log.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

…apter (#2397)

* fix(x2text): guard unstructured-url against SSRF, bound timeouts, return JSON

Validate the tenant-supplied unstructured-url with the shared
unstract.core SSRF guard. Private, loopback and link-local destinations
stay allowed by default (UNSTRUCTURED_URL_ALLOW_PRIVATE=true) so
self-hosted installs are unchanged; multi-tenant deployments set it to
false. Refusals return a 400 with a message.

Calls to Unstructured now use a finite (10s, 290s) timeout
(UNSTRUCTURED_REQUEST_TIMEOUT) and do not follow redirects. The
test-connection upstream body is returned as JSON, not text/html.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(backend): keep DB/cache and storage-path details out of API errors

The metrics health check returned str(e), naming the DB/Redis host,
port and user, to any org member; it now returns a fixed message and
logs the exception. Destination read failures no longer put the
execution-storage path in the APIException detail; the path stays in
the server log.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* chore(backend): delete unrouted duplicate internal workflow views

WorkflowExecutionInternalViewSet and FileBatchCreateInternalAPIView in
workflow_v2/views.py are copies of the routed classes in
workflow_manager/internal_views.py; nothing imports or routes them.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix: address self-review round 1 on x2text guard and error details

- Read timeout default 900s (matches the LLMWhisperer extraction wait);
  290s cut off long calls on Helm's gthread workers, which never killed
  them. nan/inf rejected; empty ALLOW_PRIVATE treated as unset.
- test-connection: upstream errors and timeouts carry detail (504 on
  timeout); process: other transport errors return 502 and mark the
  audit row Failed; refused redirects are logged.
- sample.env leaves both settings commented so code defaults apply.
- Worker copy of the HITL queue read no longer puts the storage path in
  its error.
- Commit the x2text tests as a pytest suite and register it with the rig.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): address self-review round 2

- test-connection keeps the upstream body out of detail (the key sdk1
  shows), so an allowed private destination's error page is not read
  back to the user; detail carries the status only.
- Transport-error logs record the exception type and host, not str(e),
  which carries the full URL and its query string.
- test-connection connection errors now carry detail.
- Cap UNSTRUCTURED_REQUEST_TIMEOUT at 86400s (larger values overflow).
- Raise the compose image's gunicorn --timeout to 930s so the 900s read
  timeout fires first and the 504/audit path runs; document the coupling.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): address self-review round 3

- Clamp UNSTRUCTURED_REQUEST_TIMEOUT above the 86400s sanity bound to
  the bound instead of falling back, and correct the comment (overflow
  is near 1e10, not at the cap).
- test-connection detail adds an API-key / URL-path hint for 401, 403
  and 404, still without the upstream body.
- Dockerfile: say what the 30s headroom covers, and that the single sync
  worker can be widened with GUNICORN_CMD_ARGS.
- Pin the cap boundary, RequestException breadth of log redaction, and
  the catch-all branch in tests.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* chore: move backend and workers changes to their own PR

The metrics-health, storage-path and dead-view changes now live in
#2400; this PR keeps only the x2text work.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): follow redirects per mode; correct gunicorn timeout wording

- With private destinations allowed (the docker-compose default),
  redirects are followed by requests exactly as before the guard, so OSS
  behaviour is unchanged.
- With them refused, each hop is checked with the same SSRF guard before
  it is dialled (at most 5); 301/302/303 become a body-less GET as in
  requests, 307/308 replay the rewound upload, and the API key is
  dropped on a host change.
- Dockerfile/sample.env: --timeout ends a busy sync worker (this CMD);
  the Helm chart's gthread workers are not ended by it.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): address self-review round 5

- State the block-mode requirement as a rule; no chart sets it yet.
- Refuse an unparseable redirect Location instead of a 500.
- Drop the API key on redirect by requests' own should_strip_auth rule
  (host, scheme or port change), not on hostname alone.
- Word the 401/403 hint for adapters without a key field; drop a stale
  comment; pin same-host key retention, process 303 body drop, the 403
  hint and the strip rule in tests.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): return fixed redirect-refusal messages, not exception text

CodeQL flagged str(RedirectRefused) reaching the response
(py/stack-trace-exposure, #487/#488). The caller now gets a fixed
message chosen by the exception's subclass; the detailed reason stays in
the log.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): label an unresolvable redirect hop as such; pin the log reason

A hop whose host fails DNS is transient, unlike the other refusals, so it
gets its own fixed message instead of "not allowed". Tests now assert
the detailed reason reaches the log.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* refactor(x2text): extract redirect-target check; warn on expected transport errors

SonarCloud S3776/S8572. The redirect hop's Location parsing and guard
check move into _checked_redirect_target (behaviour unchanged). An
unreachable Unstructured API is an expected outcome of a wrong adapter
URL, so it logs a warning without the traceback, which would carry the
full URL and any key in its query string.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): warn only on adapter-URL transport errors; error on egress faults

Round-9 review: SSL, proxy and mid-body failures point at this service,
not the tenant's URL, so they stay at error; connection refused, DNS and
malformed URLs log at warning. Still type and host only.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): carry cookies across checked redirect hops

With UNSTRUCTURED_URL_ALLOW_PRIVATE=false each hop was a fresh
requests.request call, so a cookie set by one hop was not sent on the
next, unlike when requests follows redirects itself. Use one Session per
post() call for the checked hops; its jar keeps cookies to the domain
that set them. Its should_strip_auth replaces the module-level session.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* feat(x2text): exempt allowlisted hosts from the private-destination check

UNSTRUCTURED_URL_ALLOWED_HOSTS lists exact hostnames (comma-separated,
trimmed, case-insensitive) that skip the private-address destination check
when UNSTRUCTURED_URL_ALLOW_PRIVATE=false. Cloud runs Unstructured in-cluster
at a Service name that resolves to a private cluster IP, so block mode would
otherwise refuse it. The allowlist applies per redirect hop, the syntactic
checks still apply, and the default is empty, so OSS behaviour is unchanged.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): log the allowlist on refusal; correct the allowlist docs

A refusal now logs UNSTRUCTURED_URL_ALLOWED_HOSTS, so an entry that can
never match (a port, a scheme, a different form of the name) is visible.
The module docstring no longer says Cloud already sets the list, states
the rollout order and exact-match consequence, scopes the resolve-twice
caveat to names that are not allowlisted, and words the per-hop rule
correctly. sample.env documents the setting, commented out.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* test(x2text): pin URL-host case folding and log order for the allowlist

Restores a mixed-case URL host check as its own test (removing it let a
host extraction that drops lower-casing pass), pins the sorted order of
the logged allowlist, and reflows an over-long docstring line.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix: refuse control characters in URL hosts and report connect timeouts as unreachable

- The shared URL guard refuses a host with a control character (NUL above
  all) as unparseable, instead of raising ValueError from inet_aton. x2text
  now returns 400 for such a URL where it returned 500, and a redirect to
  one is refused as an unparseable Location.
- x2text maps only ReadTimeout to the 504 "timed out" response. A
  ConnectTimeout is an unreachable adapter URL: 400/502, logged as a
  warning with the host. The timeout log lines now carry the host.
- Tests pin the block-mode timeout on every hop, the per-hop close(), the
  refusal log's host-only redaction, an unparseable adapter URL in block
  mode, the catch-all's traceback log, and `message` in error responses.
- Comment fixes (redirect wording, the per-hop scope of the 930s gunicorn
  timeout), a sample.env note that credentials in the URL are refused in
  either mode, and ruff 0.3.4 import order in the test files.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix: accept credentials in the Unstructured URL where private destinations are allowed

The default (allow-private) mode refused https://user:pass@host URLs, a
change from before the guard that would break a self-hosted Unstructured
behind basic auth. Where any destination is allowed the credential check
protects nothing, so the host checks now run on the URL without its
userinfo instead. Block mode still refuses credentials.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix: keep refusing credentials where removing them changes the dialled host

The userinfo-stripped copy is rebuilt the way urlparse reads the URL, which
can differ from the host urllib3 dials (http://a@b\@c/ is c to one and b to
the other). Use the copy only when urllib3 finds the same host in both;
otherwise refuse the original as credentials-in-url.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

---------

Co-authored-by: Claude Opus 5.5 <[email protected]>

…apter (#2397)

* fix(x2text): guard unstructured-url against SSRF, bound timeouts, return JSON

Validate the tenant-supplied unstructured-url with the shared
unstract.core SSRF guard. Private, loopback and link-local destinations
stay allowed by default (UNSTRUCTURED_URL_ALLOW_PRIVATE=true) so
self-hosted installs are unchanged; multi-tenant deployments set it to
false. Refusals return a 400 with a message.

Calls to Unstructured now use a finite (10s, 290s) timeout
(UNSTRUCTURED_REQUEST_TIMEOUT) and do not follow redirects. The
test-connection upstream body is returned as JSON, not text/html.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(backend): keep DB/cache and storage-path details out of API errors

The metrics health check returned str(e), naming the DB/Redis host,
port and user, to any org member; it now returns a fixed message and
logs the exception. Destination read failures no longer put the
execution-storage path in the APIException detail; the path stays in
the server log.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* chore(backend): delete unrouted duplicate internal workflow views

WorkflowExecutionInternalViewSet and FileBatchCreateInternalAPIView in
workflow_v2/views.py are copies of the routed classes in
workflow_manager/internal_views.py; nothing imports or routes them.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix: address self-review round 1 on x2text guard and error details

- Read timeout default 900s (matches the LLMWhisperer extraction wait);
  290s cut off long calls on Helm's gthread workers, which never killed
  them. nan/inf rejected; empty ALLOW_PRIVATE treated as unset.
- test-connection: upstream errors and timeouts carry detail (504 on
  timeout); process: other transport errors return 502 and mark the
  audit row Failed; refused redirects are logged.
- sample.env leaves both settings commented so code defaults apply.
- Worker copy of the HITL queue read no longer puts the storage path in
  its error.
- Commit the x2text tests as a pytest suite and register it with the rig.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): address self-review round 2

- test-connection keeps the upstream body out of detail (the key sdk1
  shows), so an allowed private destination's error page is not read
  back to the user; detail carries the status only.
- Transport-error logs record the exception type and host, not str(e),
  which carries the full URL and its query string.
- test-connection connection errors now carry detail.
- Cap UNSTRUCTURED_REQUEST_TIMEOUT at 86400s (larger values overflow).
- Raise the compose image's gunicorn --timeout to 930s so the 900s read
  timeout fires first and the 504/audit path runs; document the coupling.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): address self-review round 3

- Clamp UNSTRUCTURED_REQUEST_TIMEOUT above the 86400s sanity bound to
  the bound instead of falling back, and correct the comment (overflow
  is near 1e10, not at the cap).
- test-connection detail adds an API-key / URL-path hint for 401, 403
  and 404, still without the upstream body.
- Dockerfile: say what the 30s headroom covers, and that the single sync
  worker can be widened with GUNICORN_CMD_ARGS.
- Pin the cap boundary, RequestException breadth of log redaction, and
  the catch-all branch in tests.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* chore: move backend and workers changes to their own PR

The metrics-health, storage-path and dead-view changes now live in
#2400; this PR keeps only the x2text work.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): follow redirects per mode; correct gunicorn timeout wording

- With private destinations allowed (the docker-compose default),
  redirects are followed by requests exactly as before the guard, so OSS
  behaviour is unchanged.
- With them refused, each hop is checked with the same SSRF guard before
  it is dialled (at most 5); 301/302/303 become a body-less GET as in
  requests, 307/308 replay the rewound upload, and the API key is
  dropped on a host change.
- Dockerfile/sample.env: --timeout ends a busy sync worker (this CMD);
  the Helm chart's gthread workers are not ended by it.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): address self-review round 5

- State the block-mode requirement as a rule; no chart sets it yet.
- Refuse an unparseable redirect Location instead of a 500.
- Drop the API key on redirect by requests' own should_strip_auth rule
  (host, scheme or port change), not on hostname alone.
- Word the 401/403 hint for adapters without a key field; drop a stale
  comment; pin same-host key retention, process 303 body drop, the 403
  hint and the strip rule in tests.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): return fixed redirect-refusal messages, not exception text

CodeQL flagged str(RedirectRefused) reaching the response
(py/stack-trace-exposure, #487/#488). The caller now gets a fixed
message chosen by the exception's subclass; the detailed reason stays in
the log.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): label an unresolvable redirect hop as such; pin the log reason

A hop whose host fails DNS is transient, unlike the other refusals, so it
gets its own fixed message instead of "not allowed". Tests now assert
the detailed reason reaches the log.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* refactor(x2text): extract redirect-target check; warn on expected transport errors

SonarCloud S3776/S8572. The redirect hop's Location parsing and guard
check move into _checked_redirect_target (behaviour unchanged). An
unreachable Unstructured API is an expected outcome of a wrong adapter
URL, so it logs a warning without the traceback, which would carry the
full URL and any key in its query string.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): warn only on adapter-URL transport errors; error on egress faults

Round-9 review: SSL, proxy and mid-body failures point at this service,
not the tenant's URL, so they stay at error; connection refused, DNS and
malformed URLs log at warning. Still type and host only.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): carry cookies across checked redirect hops

With UNSTRUCTURED_URL_ALLOW_PRIVATE=false each hop was a fresh
requests.request call, so a cookie set by one hop was not sent on the
next, unlike when requests follows redirects itself. Use one Session per
post() call for the checked hops; its jar keeps cookies to the domain
that set them. Its should_strip_auth replaces the module-level session.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* feat(x2text): exempt allowlisted hosts from the private-destination check

UNSTRUCTURED_URL_ALLOWED_HOSTS lists exact hostnames (comma-separated,
trimmed, case-insensitive) that skip the private-address destination check
when UNSTRUCTURED_URL_ALLOW_PRIVATE=false. Cloud runs Unstructured in-cluster
at a Service name that resolves to a private cluster IP, so block mode would
otherwise refuse it. The allowlist applies per redirect hop, the syntactic
checks still apply, and the default is empty, so OSS behaviour is unchanged.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix(x2text): log the allowlist on refusal; correct the allowlist docs

A refusal now logs UNSTRUCTURED_URL_ALLOWED_HOSTS, so an entry that can
never match (a port, a scheme, a different form of the name) is visible.
The module docstring no longer says Cloud already sets the list, states
the rollout order and exact-match consequence, scopes the resolve-twice
caveat to names that are not allowlisted, and words the per-hop rule
correctly. sample.env documents the setting, commented out.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* test(x2text): pin URL-host case folding and log order for the allowlist

Restores a mixed-case URL host check as its own test (removing it let a
host extraction that drops lower-casing pass), pins the sorted order of
the logged allowlist, and reflows an over-long docstring line.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix: refuse control characters in URL hosts and report connect timeouts as unreachable

- The shared URL guard refuses a host with a control character (NUL above
  all) as unparseable, instead of raising ValueError from inet_aton. x2text
  now returns 400 for such a URL where it returned 500, and a redirect to
  one is refused as an unparseable Location.
- x2text maps only ReadTimeout to the 504 "timed out" response. A
  ConnectTimeout is an unreachable adapter URL: 400/502, logged as a
  warning with the host. The timeout log lines now carry the host.
- Tests pin the block-mode timeout on every hop, the per-hop close(), the
  refusal log's host-only redaction, an unparseable adapter URL in block
  mode, the catch-all's traceback log, and `message` in error responses.
- Comment fixes (redirect wording, the per-hop scope of the 930s gunicorn
  timeout), a sample.env note that credentials in the URL are refused in
  either mode, and ruff 0.3.4 import order in the test files.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix: accept credentials in the Unstructured URL where private destinations are allowed

The default (allow-private) mode refused https://user:pass@host URLs, a
change from before the guard that would break a self-hosted Unstructured
behind basic auth. Where any destination is allowed the credential check
protects nothing, so the host checks now run on the URL without its
userinfo instead. Block mode still refuses credentials.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* fix: keep refusing credentials where removing them changes the dialled host

The userinfo-stripped copy is rebuilt the way urlparse reads the URL, which
can differ from the host urllib3 dials (http://a@b\@c/ is c to one and b to
the other). Use the copy only when urllib3 finds the same host in both;
otherwise refuse the original as credentials-in-url.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

---------

Co-authored-by: Claude Opus 5.5 <[email protected]>
(cherry picked from commit 86a63b8)