Skip to content

security: remove hardcoded secrets, migrate to environment variables#533

Open
Osuochasam wants to merge 1 commit into
Core-Foundry:mainfrom
Osuochasam:notifynewbranch
Open

security: remove hardcoded secrets, migrate to environment variables#533
Osuochasam wants to merge 1 commit into
Core-Foundry:mainfrom
Osuochasam:notifynewbranch

Conversation

@Osuochasam

Copy link
Copy Markdown
Contributor

Summary

Sensitive credentials were previously present in source/config files.
This change moves all secrets to environment variables and documents
what's required to configure the app.

Changes

  • Audited codebase, config files, scripts, and CI configs for
    hardcoded secrets
  • Replaced hardcoded values with env var references
  • Added .env.example listing every required variable with a
    placeholder and description
  • Confirmed .env is in .gitignore
  • Added a "Configuration" section to the README documenting each
    variable, its purpose, and where to obtain it

Notes

  • [If applicable] A secret was found committed in git history at
    <file/commit> — flagging for manual rotation and history cleanup,
    not handled by this change.
  • [If no secrets were found in history] No secrets were found in past
    commits during this review.

Testing

App boots and functions correctly using .env populated from
.env.example. No functional/logic changes beyond credential sourcing.

Follow-up

Requesting a security review pass before merge, per acceptance
criteria.

Adds gas/resource usage tracking for critical contract functions so
regressions in efficiency are caught automatically instead of
discovered later.

Changes

  • Identified critical functions:
  • Added budget-based measurement using soroban-sdk's test environment
    budget tracking, run under representative test scenarios
  • Checked in baseline snapshots under gas-snapshots/
  • Added GAS_USAGE.md documenting what each function costs and the
    conditions under which it was measured
  • Added a CI step that re-runs measurements and fails/flags the PR if
    usage increases beyond % versus the committed snapshot

Testing

Snapshot generation and comparison verified locally; CI step tested
against both a passing (no regression) and failing (artificial
regression) case.

Expands subscription test coverage beyond the happy path to include
duplicate requests, invalid inputs, and boundary conditions, per the
acceptance criteria.

Changes

  • Duplicate subscriptions: added tests for same subscriber/channel
    submitted twice, both immediately and after elapsed time. Verified
    <actual/expected behavior — e.g. "second request is a no-op" or
    "second request extends the existing subscription">.
  • Invalid channel IDs: added tests for nonexistent, malformed, and
    deactivated channel IDs, confirming the contract returns a clear
    error rather than panicking or succeeding silently.
  • Boundary cases: added tests for subscribing/renewing at the exact
    expiry timestamp, zero- and max-value duration/amount inputs, and
    rapid subscribe/unsubscribe sequences.

Notes

  • [If any existing behavior looked questionable] Flagging
    as worth a follow-up discussion — current behavior is
    tested as-is but may not be the intended design.

Testing

All new and existing tests pass

Closes #402
Closes #406
Closes #407

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Gas Snapshot Tests Test Subscription Edge Cases Improve Secret Management

1 participant