[MOSIP-44447] Secure Docker tokens using dynamic GitHub secrets instead of user input#1583
[MOSIP-44447] Secure Docker tokens using dynamic GitHub secrets instead of user input#1583bhumi46 wants to merge 11 commits intomosip:release-1.2.0.1from
Conversation
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <111699703+bhumi46@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
release/vidivi/transfer_report.md (2)
1-20:⚠️ Potential issue | 🟠 MajorTest run artifacts — revert before merging.
This report reflects a personal test run (org
bn46, imageuitest-verify). The previous report contained actual MOSIP transfer data (mosipqa/partner-onboarder). Likeimages.txt, this file should be reverted to avoid overwriting production transfer records with test data.
12-12:⚠️ Potential issue | 🟡 MinorMinor:
00instead of0for failed transfers.This is likely generated by the reporting script, but
00looks like a formatting bug in the report generator rather than the intended value0.
🤖 Fix all issues with AI agents
In @.github/workflows/image-transfer.yml:
- Around line 55-74: The validation step is using the unescaped
steps.ORG_TOKEN.outputs.TOKEN directly inside the shell script which creates an
injection vector; change the job to pass the token output via the step's env
(e.g., set env: SECRET_NAME: ${{ steps.ORG_TOKEN.outputs.TOKEN }} and
TOKEN_EXISTS: ${{ secrets[steps.ORG_TOKEN.outputs.TOKEN] != '' }}), then
reference $SECRET_NAME and $TOKEN_EXISTS inside the run block (avoid
interpolating ${{ ... }} in the script body); update the Validate secret
configuration step to read the secret name from the env variables (SECRET_NAME
and TOKEN_EXISTS) instead of inline workflow expressions to harden against
injection and pair this with the earlier fix to the ORG_TOKEN output generation.
- Around line 42-53: The step is vulnerable because it injects the workflow
input directly into the run block via ORG="${{ inputs.DESTINATION_ORGANIZATION
}}"; instead, pass the user-controlled value through the step env (e.g. set env
DESTINATION_ORGANIZATION: ${{ inputs.DESTINATION_ORGANIZATION }}) and reference
the safe shell variable (ORG="$DESTINATION_ORGANIZATION") inside the run; also
fully quote variable expansions, sanitize when building ORG_UPPER with a safe
formatter (use printf '%s' "$ORG" before piping to tr), construct TOKEN_SECRET
from the quoted ORG_UPPER, and write the output using a safe printer (printf) to
GITHUB_OUTPUT to avoid shell interpretation—update references to ORG, ORG_UPPER,
and TOKEN_SECRET accordingly.
In `@release/vidivi/images.txt`:
- Line 1: The images.txt change replaces production entries with a personal test
image (injistackdev2/uitest-verify@sha256:b14db85...) and should be reverted;
restore the original MOSIP project image lines (e.g., injistackdev2/inji-web and
injistackdev2/mimoto) in images.txt and remove or move the uitest-verify test
entry into a separate test-specific file or branch to avoid losing production
image entries.
In `@release/vidivi/README.md`:
- Around line 202-231: The secret-name generation currently uppercases org names
using tr '[:lower:]' '[:upper:]' but does not convert hyphens to underscores,
producing invalid names for orgs like "my-org"; update the workflow step that
builds the secret name so it first maps hyphens to underscores (e.g., replace
'-' with '_') and then uppercases the result (replace the existing tr pipeline),
and update release/vidivi/README.md to document this mapping and add an example
showing "my-org" → "MY_ORG_DOCKER_TOKEN" plus a note about allowed characters
([A-Z0-9_]).
- Remove TOKEN from user inputs, use dynamic GitHub secrets only - Implement org-based secret selection (<ORG>_DOCKER_TOKEN) - Fix shell injection vulnerabilities using env block and quoted variables - Replace echo with printf for safe output handling - Add secret validation with secure boolean checks - Support hyphenated org names (convert hyphens to underscores) - Update documentation with security best practices and examples Signed-off-by: bhumi46 <thisisbn46@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/image-transfer.yml:
- Around line 47-56: Validate DESTINATION_ORGANIZATION before normalization:
ensure it is non-empty and contains only allowed characters (letters, numbers
and hyphens) and reject any value with dots, spaces or other special characters;
perform this check before computing ORG_NORMALIZED and TOKEN_SECRET, and if
invalid fail the job with a clear error message indicating the org name is
invalid (not "secret missing"). Reference DESTINATION_ORGANIZATION,
ORG_NORMALIZED and TOKEN_SECRET when making the change so the validation runs
prior to the tr-based normalization and the subsequent printing of TOKEN.
- Add SECRET_NAME optional input parameter - Support both dynamic (<ORG>_DOCKER_TOKEN) and custom secret names - Fallback to dynamic naming when SECRET_NAME is empty - Add logging to show which secret name is being used - Maintain backward compatibility with existing workflows Signed-off-by: bhumi46 <thisisbn46@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/image-transfer.yml (1)
101-105: Minor: inconsistent quoting on secret references.Line 103 wraps the expression in double quotes while lines 104–105 do not. This is functionally harmless but inconsistent.
Proposed fix
- TOKEN: "${{ secrets[needs.chk_token.outputs.TOKEN] }}" + TOKEN: ${{ secrets[needs.chk_token.outputs.TOKEN] }}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
release/vidivi/transfer_report.md (1)
12-12: Minor formatting suggestion: simplify "00" to "0".The failed transfers count shows "00" which could be simplified to "0" for consistency with typical numeric formatting.
Minor formatting improvement
-- **Failed transfers**: 00 +- **Failed transfers**: 0
- Validate that DESTINATION_ORGANIZATION is non-empty - Ensure only letters, numbers, and hyphens are allowed - Reject dots, spaces, and other special characters - Perform validation before normalization and token secret computation - Provide clear error messages for invalid organization names Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <111699703+bhumi46@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores