Repository navigation
Align Fourfront dependencies and harden uploads, exports, and deploys - #1936
Open
willronchetti wants to merge 14 commits into
Open
willronchetti wants to merge 14 commits into
willronchetti wants to merge 14 commits into
Conversation
…efficiency + security, test & deploy hardening Implements the justified, Fourfront-only alignment fixes from the two comparative audits (snovault 11.35.2 and SMaHT Portal), verified against current Fourfront source. dcicsnovault 11.27.0 -> 11.35.2, dcicutils 8.18.3 -> 8.18.8 (lock regenerated): - Delivers the #335 nested-linkTo invalidation-scope correctness fix (stale-ES bug on Fourfront's array-of-object linkTo embeds), #333 MAX(sid) indexing hoist, #324 ES access-pattern efficiency, and attachment/JWT/access-key security fixes -- all in modules Fourfront imports from snovault, so they arrive purely via the bump. - All 133 actively-used `from snovault...` import paths verified intact against 11.35.2; removed transitive pmdarima sub-tree (pandas/scipy/sklearn/...) is unused by Fourfront; numpy/pytz/xlrd retained. Search efficiency (local fork src/encoded/search.py; ports of snovault #318): - get_all_subsequent_results: drop aggregations and disable track_total_hits on the 2nd..Nth page of a limit=all scan (they are only read from page 1). - list_source_fields: object/raw frames no longer pull the discarded embedded.* blob into _source. - set_facets: skip default facet aggregation construction when frame != embedded (format_facets discards them); custom/schema aggregations still applied. batch_download hardening: - Neutralize CSV/TSV formula injection (CWE-1236) for cells beginning with = + - @ across metadata_tsv, report_download and peak_metadata. - report_download now bounds the ES _source to exactly the rendered column paths instead of fetching every embedded field (metadata_tsv already bounded fields). Test reliability: - workbook fixture re-indexes until DB/ES counts agree (root-causes flakiness masked by --force-flaky), with a bounded retry cap. - Replace deprecated @pytest.yield_fixture (removed in pytest 8) with @pytest.fixture in conftest.py, test_fixtures.py, test_indexing.py. - CI: add `wipe-test-indexer-queues $TEST_JOB_ID` cleanup (now available in snovault 11.35.2) so per-run SQS indexer queues don't accumulate. Docker/build/runtime (safe, offline-validated subset): - Supervise nginx under supervisord (foreground) instead of `service nginx start`; fail-fast `set -e` in entrypoint_deployment.bash. - nginx: bounded proxy_next_upstream_tries/timeout; enlarge upstream `zone app` 32k -> 1m (modern nginx rejects the undersized zone); add `nginx -t` build gate. - Pin ES image opensearchproject/opensearch:2.6.0 and CI aws-actions/configure-aws-credentials@main -> @v4. Focused regression tests (ES-free, pass against snovault 11.35.2): - test_search_query_shape.py, test_batch_download_unit.py, test_indexer_auth_debug_config.py. Deferred (documented, not shipped): multi-stage Dockerfile + BuildKit buildspec (needs `docker build` validation, unavailable offline); staggered/selective reindex (gated on reconciling loadxl_order() with ITEM_INDEX_ORDER); clear-db-es-contents/memlimit fork retargeting; snovault PR 330 beta; search.py/batch_download.py fork retirement; pre-existing read_single_sheet dev-CLI breakage.
aschroed
reviewed
Jul 28, 2026
aschroed
left a comment
aschroed
left a comment
Member
There was a problem hiding this comment.
Trying to deploy local and seeing this issue with 'document' insert.
webtest.app.AppError: Bad response: 422 Unprocessable Entity (not 200 OK or 3xx redirect for http://localhost/dcf15d5e-40aa-43bc-b81c-32c70c9afb48)
b'{"@type": ["ValidationFailure", "Error"], "status": "error", "code": 422, "title": "Unprocessable Entity", "description": "Failed validation", "errors": [{"location": "body", "name": ["attachment", "href"], "description": "Incorrect file type. (Appears to be application/octet-stream)"}]}'
`external_creds()` in `src/encoded/types/file.py` minted scoped, temporary S3
credentials with `sts:GetFederationToken`. AWS only accepts that call from an
IAM user holding long-lived access keys and rejects it outright when the caller
itself holds temporary credentials, so it cannot work for any role-based caller.
Trigger: 8.9.2 moved the GitHub Actions build to OIDC-retrieved AWS credentials
(`.github/workflows/main.yml` assumes `4dn-dcic-github-actions-deployment-role`
with `id-token: write`), and the containerized deployment runs under a task role.
Both hold session credentials.
Masking condition: the `IDENTITY` branch bypassed the ambient chain and passed
`S3_AWS_ACCESS_KEY_ID`/`S3_AWS_SECRET_ACCESS_KEY` read out of the application
configuration secret, so federation kept working exactly as long as static IAM
user keys stayed in that secret. In tests it is masked twice over: the repo-root
`conftest.py` always sets `IDENTITY`, and the pre-existing
`test_file.py::test_external_creds` mocks `boto3` wholesale without asserting the
mechanism, so it passes under either implementation.
Symptom once those keys are gone: STS returns AccessDenied ("Cannot call
GetFederationToken with session credentials"). Uploads fail at `File.create`,
`File._update` (including every `extra_files` entry) and `POST @@upload`;
`@@download` fails too, because it falls back to `build_external_creds` whenever
a File has no stored `external` propsheet -- true for every File created in a
status outside ('uploading', 'to be uploaded by workflow', 'upload failed') and
for extra_files missing their per-format propsheet. `upload_key` silently
degrades to a "Failed to acquire upload credentials" string instead.
Both flows funnel through the single `external_creds()`, so one mechanism change
covers upload and download; `encoded-core` 1.0.0 (smaht-dac#26) and cgap-portal
already landed the same migration.
- Call `sts:AssumeRole` on the ambient credential chain with no access keys, and
map `AssumedRoleUser.Arn`/`AssumedRoleId` in place of `FederatedUser`.
- Read the role from `S3_UPLOAD_ROLE_ARN`, identity first and environment as
fallback. The fallback deviates from encoded-core deliberately: Fourfront's
root `conftest.py` always sets `IDENTITY`, so an identity-only lookup leaves
the environment unreachable in every test and CI run.
- Session policy is byte-identical, so the authorization boundary is unchanged:
still `s3:PutObject` on the one target key, now intersected with the assumed
role's own permissions. No KMS statement was ported; that would broaden it.
- `upload_key` also catches `BotoCoreError`, because a missing role ARN fails
client-side as `ParamValidationError` (a `BotoCoreError`, not a `ClientError`)
-- a failure mode GetFederationToken could not produce -- and would otherwise
turn a masked message into a 500.
Stated deltas, not oversights: credential lifetime drops from the
GetFederationToken default of 12h to the AssumeRole default of 1h. No
`DurationSeconds` is set, matching encoded-core, since a value above the role's
`MaxSessionDuration` hard-fails; the `POST @@upload` endpoint already exists to
reissue credentials. `encoded-core`'s `upload=True` parameter is not ported --
its `f'{upload_or_download}_credentials'` keys would break Fourfront's
`upload_credentials()`, `extra_files_creds()` and `post_upload`, which read the
literal names. `S3_UPLOAD_ROLE_ARN` is not added to `.github/workflows/main.yml`:
the repo has no `AWS_OIDC_ROLE_ARN` secret, and whether the hardcoded OIDC
deployment role permits self-assumption is not verifiable offline. Provisioning
the role into the application configuration identity is the infra prerequisite.
Regression coverage in `src/encoded/tests/test_file_upload_credentials.py`
(15 tests, ES/Postgres/AWS-free, boto3 mocked): AssumeRole is called and
GetFederationToken is not, on both the upload entry point and the
`build_external_creds` path the download view uses; no access keys reach boto3;
the session policy is exactly PutObject on the target key; AssumedRoleUser
mapping and the public `upload_credentials`/`upload_url` key names; the
`name=None` link-only path still mints nothing; role-ARN sourcing across
identity, environment, the identity-plus-environment fallback and the
missing-everywhere case; presigned download URLs still use the ambient chain;
and `upload_key` degrades rather than raising for both `ParamValidationError`
and `ClientError`. Reverting the mechanism fails 6 of them; reverting the
`except` clause fails another.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Use the registered [working, unit] pytestmark that this PR's other new ES-free modules (test_search_query_shape, test_batch_download_unit, test_indexer_auth_debug_config) already use, instead of the deprecated 'setone' marker. The module carries no 'workbook' marker, so it is selected by the UNIT job's -m "working and ... and not workbook". Verified against the real pytest.ini addopts (datafixtures/serverfixtures plugins + --instafail loaded), not just -o addopts="": 15 passed. Also drops a stray f-prefix on a literal with no placeholders. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
willronchetti
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
dcicsnovaultto 11.35.2 anddcicutilsto 8.18.8; retain Fourfront-specific schemas, facets, authorization, and workbook reconciliation.PutObjectAssumeRole credentials with valid session labels. Downloads never mint upload credentials; upload-key calculation needs no AWS calls.Reported document 422
The exact
photo.zipinsert fails on both master and this PR with native libmagic 5.46: loadxl detects the file as ZIP, but buffer validation sees octet-stream. Native 5.45 masks it; 5.47 accepts the identical archive, including when only the native library is replaced. The dependency bump did not change this MIME-validation path. No MIME allowlist or fixture content was weakened; startup now reports an actionable dependency error. Evidence/remediation: attachment-mime.md.Validation
Deployment prerequisites / boundaries
S3_UPLOAD_ROLE_ARNthrough identity/environment or the CI repository variable. Live IAM trust/permissions remain operator validation, not a self-assumption fallback.