Implemented Subdirectory Specification for Source#78
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to allow specifying a subdirectory for skills during initialization. The changes involve adding a new prompt function useSourceSubdir, and adjusting the performFreshInit logic to handle the subdirectory and save the configuration after all paths are determined. The implementation is mostly correct, but I've found a potential security vulnerability in how user input for the subdirectory is handled. My review includes a suggestion to sanitize this input to prevent path traversal attacks and also improves input reading to be more robust.
runkids
left a comment
There was a problem hiding this comment.
Thanks for the PR! The subdirectory prompt is a great addition — merging this now.
I'll follow up with a few tweaks in a subsequent commit:
- Input validation (path traversal prevention, e.g.
..,/) - TTY check so it doesn't block in non-interactive/CI environments
--subdirCLI flag for scriptable usage- Align prompt style with the existing
uipackage helpers
Appreciate the contribution 🙏
|
Good points on the cli flag and UI, will just say that I don't consider the path traversal as having real security risk as the only impact is the user intentionally shooting themselves in the foot. |
|
That makes sense for the interactive prompt! Quick question — we're planning to add a |
…i helpers Follow-up to #78. Adding: - TTY guard so the prompt is skipped in non-interactive/CI environments - --subdir CLI flag for scriptable usage - Consistent use of bufio.Reader (no mixed fmt.Scanln) - ui.Info / ui.Success for prompt output aligned with other init steps
…i helpers Follow-up to #78. Adding: - TTY guard so the prompt is skipped in non-interactive/CI environments - --subdir CLI flag for scriptable usage - Consistent use of bufio.Reader (no mixed fmt.Scanln) - ui.Info / ui.Success for prompt output aligned with other init steps
|
That's a fair point — agreed that for an interactive prompt, path traversal isn't a real security concern since the user is only affecting their own environment. Following your suggestion, we won't be adding path traversal validation. #79 focuses on the other improvements: TTY guard, |
…i helpers Follow-up to #78. Adding: - TTY guard so the prompt is skipped in non-interactive/CI environments - --subdir CLI flag for scriptable usage - Consistent use of bufio.Reader (no mixed fmt.Scanln) - ui.Info / ui.Success for prompt output aligned with other init steps
|
@runkids While you can if you want, and it definitely doesn't hurt, in the current state of the program, I cannot identify a threat scenario where this can be leveraged. |
|
@daylamtayari Thanks for the thorough security assessment — it really helped us scope the follow-up in #79. Great collaborating with you! 🙌 |
Add a prompt to the
initcommand to allow users to specify a subdirectory as the source, allowing users to be able to store their skills in askills/directory in their repo and not the root. Resolves #74Changelog:
useSourceSubdirfunction that prompts the user and returns a string that is non-empty if the user specifiedinitGitIfNeededandsetupGitRemotedo not rely on the configuration.There is a caveat that this introduces where if a user already initialised but did not setup a repository and then runs
skillshare init --remote REMOTEURL, it will initialise the git repository and remote in the subdirectory and not the parent directory. This is due to the program not having knowledge that it is a subdirectory and not the parent.I think this is an acceptable edge case as for a user to perform that, they already did not follow the recommended guidance and then are establishing the remote after the fact and I would want to expect such a user to check that it is in the appropriate dir if they care about the skills being in the source subdir. The fix would also just be moving the
.git/directory up a level which is minimal effort.