Enable using read-only HTTPS repositories #2171
Conversation
If someone wants to clone read-only from an `https://github.com/...` URL, that should be permitted, and in that case they shouldn't be required to provide an SSH key.
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in the pull request focus on the Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
spiffworkflow-backend/bin/clone_process_models (1)
33-35: LGTM! Conditional SSH configuration enables HTTPS supportThis change correctly makes SSH configuration optional, allowing for both SSH and HTTPS repository access. Consider adding a comment to explain this dual-mode support:
+# Only configure SSH if a private key is provided, otherwise allow HTTPS access if [[ -n "${SPIFFWORKFLOW_BACKEND_GIT_SSH_PRIVATE_KEY_PATH}" ]]; then export GIT_SSH_COMMAND="ssh -F /dev/null -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -i ${SPIFFWORKFLOW_BACKEND_GIT_SSH_PRIVATE_KEY_PATH}" fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
spiffworkflow-backend/bin/clone_process_models(1 hunks)
🔇 Additional comments (2)
spiffworkflow-backend/bin/clone_process_models (2)
27-28: LGTM! Improved variable assignment clarity
The separation of variable assignment and export improves code readability while maintaining the same functionality.
41-41: Verify HTTPS cloning functionality
The implementation looks correct, but let's verify there are no hardcoded SSH URLs or forced SSH configurations in the codebase that might prevent HTTPS cloning.
✅ Verification successful
HTTPS cloning is properly supported
The implementation correctly handles both SSH and HTTPS cloning:
- SSH configuration (
GIT_SSH_COMMAND) is only set when an SSH key is provided - The clone URL is controlled by
SPIFFWORKFLOW_BACKEND_GIT_PUBLISH_CLONE_URLenvironment variable - No forced SSH configurations that would prevent HTTPS cloning
- The hardcoded SSH URLs found are only in development-related files (package-lock.json and deployment scripts) and don't affect the cloning functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential hardcoded git URLs or SSH configurations that might interfere with HTTPS cloning
echo "Checking for hardcoded git URLs:"
rg -i "git@github|ssh://git@" --type bash --type python
echo "Checking for forced SSH configurations:"
rg -i "GIT_SSH_COMMAND|StrictHostKeyChecking" --type bash --type python
Length of output: 353
Script:
#!/bin/bash
# Search for potential hardcoded git URLs or SSH configurations that might interfere with HTTPS cloning
# Using more generic file type options
echo "Checking for hardcoded git URLs:"
rg -i "git@github|ssh://git@"
echo "Checking for forced SSH configurations:"
rg -i "GIT_SSH_COMMAND|StrictHostKeyChecking"
# Also check the specific clone script for any git clone related commands
echo "Checking clone script implementation:"
cat spiffworkflow-backend/bin/clone_process_models
Length of output: 3397
| @@ -24,18 +24,23 @@ fi | |||
|
|
|||
| if [[ -z "${SPIFFWORKFLOW_BACKEND_GIT_SSH_PRIVATE_KEY_PATH:-}" ]]; then | |||
| if [[ -n "${SPIFFWORKFLOW_BACKEND_GIT_SSH_PRIVATE_KEY:-}" ]]; then | |||
| export SPIFFWORKFLOW_BACKEND_GIT_SSH_PRIVATE_KEY_PATH=$(mktemp /tmp/ssh_private_key.XXXXXX) | |||
| SPIFFWORKFLOW_BACKEND_GIT_SSH_PRIVATE_KEY_PATH=$(mktemp /tmp/ssh_private_key.XXXXXX) | |||
There was a problem hiding this comment.
CodeRabbit is missing a subtlety here that shellcheck.net caught: If you export on the same line that you run a subshell, you might mask an error in the execution of the subshell. Not likely here with mktemp, but good BASH style says the export should happen on a separate line from the assignment for that reason.
If someone wants to clone read-only from an
https://github.com/...URL, that should be permitted, and in that case they shouldn't be required to provide an SSH key. (Without this, if you provide neither the path nor the key, then you seeERR /app/bin/clone_process_models: line 39: SPIFFWORKFLOW_BACKEND_GIT_SSH_PRIVATE_KEY_PATH: unbound variablein the logs and the clone on the penultimate line never happens.)Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Refactor