SkillSpector flags critical risks — requesting clarification on specific findings #215
Replies: 5 comments 1 reply
|
Good precaution, and the flagged lines deserve exact answers. I checked each finding against the code at HEAD before replying. 1. 2. 3. 4. On the 100/100 CRITICAL: a scanner that weights "reads env keys + makes HTTP calls" as critical rates every API-backed tool DO-NOT-INSTALL - SkillsLLM finding nothing high-severity on the same tree is the same disagreement from the other side. For actual supply-chain assurance: commits are signed, OSSF Scorecard runs on this repo, CI is public, and the telegram integration is one readable file. If any finding ever names a sink that is NOT one of the vendor APIs above, that is a real report - file it as a security issue and I will treat it as one. |
|
Thanks — this is exactly the kind of answer I was hoping for, and it changes my assessment of the project meaningfully. The vendor-only endpoint breakdown, the pinned --allowedTools on the background agent, signed commits, public CI, OSSF Scorecard — that's a real security posture, not just a README claim. I'm no longer treating the SkillSpector 100/100 as representative of actual risk.
On the two YARA matches: agreed, confirmed false positives on my end too, independently. I had a second model (Codex CLI) audit the repo at HEAD (commit ce2b5e8) specifically trying to disprove that conclusion rather than confirm it, and it reached the same reading on both — telegram_journal.py:48 is a local config-to-env loader with no network call on that line, and architecture.md:159 is prose that explicitly denies shipping cron/launchd artifacts.
You invited exactly this: "if any finding ever names a sink that is NOT one of the vendor APIs above, that is a real report — file it." That same deeper audit surfaced three things outside the scanner's scope entirely — not credential exfiltration, but real operational risk I think are worth your attention regardless of how they get filed:
1. No allowlist on the Telegram integration. I don't see a check against a chat_id or user ID before the bot processes an update. If that's accurate, anyone who messages the bot can trigger vault writes, spend your API quota, and — this is the part that worries me most — feed content into a vault an agent later treats as trusted memory for decisions. That's a different risk class than "credentials sent to the wrong place": it's indirect influence over what the agent believes and does later, from an unauthenticated sender. If there's already a gate I'm missing (maybe enforced elsewhere, or via .env), I'd genuinely like to know where.
2. Possible path traversal in create_stub(). The note filename looks like it's derived from LLM output without an explicit check that the resolved path stays inside the vault root (no visible ../absolute-path rejection or resolved.is_relative_to(vault)check). Might already be handled upstream and I'm missing it — flagging in case it's not.
3. Lockfile CVEs with public advisories, not just version drift: cryptography in the range affected by the OpenSSL wheel advisory (GHSA-537c-gmf6-5ccf, fixed in 48.0.1+) and urllib3 before the fix for the streaming decompression-bomp bypass (GHSA-mf9v-mfxr-j63j, fixed in 2.7.0). Given the toolkit makes a lot of outbound requests to external URLs/feeds, the urllib3 one seemed worth naming specifically rather than leaving as generic "outdated dependency" noise.
None of these are exfiltration and I'm not claiming malicious intent anywhere in this repo — the response above makes that reading hard to sustain, honestly. But #1 and #2 seem like real gaps regardless of vendor-endpoint discipline, and #3 is concrete enough to action.
Given the quality of this response, my plan is: hold off on Telegram and the background agent specifically until #1 and #2 are addressed or clarified, but I'm a lot more open to running the core (no Telegram, no scheduled agents, pinned commit) than I was before this thread. Want me to open these as separate issues, or is this thread enough for your tracking?
… Il giorno 19 ago 2026, alle ore 22:32, Eugeniu Ghelbur ***@***.***> ha scritto:
Good precaution, and the flagged lines deserve exact answers. I checked each finding against the code at HEAD before replying.
1. telegram_journal.py:48 - "credential_exfiltration_webhook": false positive. Line 48 is a config loader: it reads your own ~/.config file and puts its keys into os.environ on your machine. The file's outbound endpoints are exactly three, all first-party: api.telegram.org (a Telegram bot cannot function without sending its token there), api.openai.com (Whisper transcription, only if you configured that key), api.anthropic.com. Each key goes only to its own vendor. No webhook, no third-party sink. The rule matches the shape "env credentials + HTTP calls" - which is also what the 7 TT3 findings are: Perplexity/Grok/Gemini keys sent to Perplexity/Grok/Gemini. That is what a research toolkit is.
2. architecture.md:159 - "backdoor_persistence": false positive. Line 159 is prose - the "Scheduled agents" section of a documentation file, which explicitly says the skill ships NO cron/launchd artifacts ("a usage pattern, not a set of bundled cron files"). The rule matched the words cron and launchd in docs.
3. curl | bash (Mondoo): real, standard, optional. The script fetched is scripts/quick-install.sh in this repo, readable before running. Alternatives are documented: clone + install.sh, or the Claude Code plugin marketplace.
4. --dangerously-skip-permissions (Mondoo): real, and the README says so in bold. That is the optional background agent. It ships INERT and is opt-in precisely because unattended writes need that flag. Armed, its tool surface is pinned with --allowedTools "Read,Write,Edit,Glob,Grep" - no Bash, no network - and it only adds/updates notes. README's "What arming it costs" section is the honest version of this trade; leave it off and everything else works.
On the 100/100 CRITICAL: a scanner that weights "reads env keys + makes HTTP calls" as critical rates every API-backed tool DO-NOT-INSTALL - SkillsLLM finding nothing high-severity on the same tree is the same disagreement from the other side. For actual supply-chain assurance: commits are signed, OSSF Scorecard runs on this repo, CI is public, and the telegram integration is one readable file.
If any finding ever names a sink that is NOT one of the vendor APIs above, that is a real report - file it as a security issue and I will treat it as one.
—
Reply to this email directly, view it on GitHub <#215?email_source=notifications&email_token=CL5JVIJGKASMIRDXCUYS33L5KYFGXA5CNFSNUABIM5UWIORPF5TWS5BNNB2WEL2ENFZWG5LTONUW63SDN5WW2ZLOOQXTCOBQHA2DIMJQUZZGKYLTN5XKMYLVORUG64VFMV3GK3TUVRTG633UMVZF6Y3MNFRWW#discussioncomment-18084410>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/CL5JVIMD53L7GWB5U5JSCXT5KYFGXAVCNFSNUABJKJSXA33TNF2G64TZHMYTCOJQGU2TONJUGU5UI2LTMN2XG43JN5XDWMJQGY2DENZQGSQXMAQ>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/CL5JVIN7Q5SK4ZDAS57VQGT5KYFGXA5CNFSNUABIM5UWIORPF5TWS5BNNB2WEL2ENFZWG5LTONUW63SDN5WW2ZLOOQXTCOBQHA2DIMJQUZZGKYLTN5XKMYLVORUG64VFMV3GK3TUVJTG633UMVZF62LPOM> and Android <https://github.com/notifications/mobile/android/CL5JVIPNIIRVTMHUGENAO3T5KYFGXA5CNFSNUABIM5UWIORPF5TWS5BNNB2WEL2ENFZWG5LTONUW63SDN5WW2ZLOOQXTCOBQHA2DIMJQUZZGKYLTN5XKMYLVORUG64VFMV3GK3TUVZTG633UMVZF6YLOMRZG62LE>. Download it today!
You are receiving this because you authored the thread.
|
|
@robertcamero all three were real, and all three are now fixed on 1. Telegram allowlist - confirmed, fixed. You were not missing a gate; there was none. 2. 3. Lockfile advisories - bumped. Tests for 1 and 2 are in On your question: this thread was enough for tracking, and it is now the reference for all three. If you re-run the SkillSpector / Codex pass against the new HEAD and anything else names a real sink or an unbounded write, file it as an issue and I will treat it the same way. |
|
Hi Eugeniu,
Thanks for the detailed response — I really appreciate the care you put into all three points, especially making the Telegram allowlist fail closed and fixing the path traversal so quickly. Pointing to the actual commits and adding dedicated tests makes it verifiable rather than just a reassurance in words, which is exactly the kind of maintainer response that builds real trust in a project.
For context: those three points came out of a multi-model review process I ran before deciding whether to adopt obsidian-second-brain (Claude, Codex CLI, and Gemini cross-checking each other's analysis). Your response closes that loop properly. I'm holding off on adoption for now — not out of any remaining concern, but because I want to finish a real usage cycle with my current stack first, as the review had recommended. If that cycle surfaces something concrete later, I'll open a proper issue instead of reviving this thread.
Really solid work — thanks again for the time you put into it.
Robert
… Il giorno 26 ago 2026, alle ore 23:37, Eugeniu Ghelbur ***@***.***> ha scritto:
@robertcamero <https://github.com/robertcamero> all three were real, and all three are now fixed on main in fd4a963 <fd4a963>. Sorry for the week of silence on this one; the deeper audit was the useful part of the thread and it deserved a faster answer. Details per item, so you can verify against the code rather than take my word for it:
1. Telegram allowlist - confirmed, fixed. You were not missing a gate; there was none. telegram_journal.py processed every update getUpdates returned. There is now a required TELEGRAM_ALLOWED_CHAT_IDS (comma-separated chat ids) checked before any handler runs, and it fails closed: an unlisted sender is refused and logged to stderr, never processed, and once a list exists strangers get no reply at all. With the variable unset the bot processes nothing; the one thing it does is tell the sender their own chat id so the owner can finish setup. setup.sh prompts for it, the env template and README document it, and there is an upgrade note in the CHANGELOG because existing installs stop saving until the id is set. Your framing of the risk (indirect influence over what the agent later treats as trusted memory, not credential theft) is the one I put in the code comment and the CHANGELOG, because it is the right one for an AI-first vault.
2. create_stub() path traversal - confirmed, fixed. It built folder / f"{name}.md" with name taken straight from a [[wikilink]] the model wrote, and nothing upstream checked it. Names now go through safe_note_path(): path separators, .., dot-names and NUL are refused, and the resolved parent must be exactly the target folder. A refusal logs and skips the stub; the capture itself still lands in the daily note. I checked the other model-derived filenames while I was there: the image and PDF paths were already date-prefixed and stripped to [A-Za-z0-9._-], so they were not reachable, and are unchanged.
3. Lockfile advisories - bumped. cryptography 47.0.0 -> 50.0.1 and urllib3 2.6.3 -> 2.7.0. Both are transitive via google-api-python-client, so uv.lock only. Agreed that the urllib3 one mattered specifically here given how much the research toolkit fetches from arbitrary URLs.
Tests for 1 and 2 are in tests/test_telegram_ingest.py: empty list refuses everyone, listed id passes, unlisted id never reaches a handler and gets no reply, owner gets their id when nothing is configured; traversal, absolute path and dot-names are refused, plain and Unicode names still write.
On your question: this thread was enough for tracking, and it is now the reference for all three. If you re-run the SkillSpector / Codex pass against the new HEAD and anything else names a real sink or an unbounded write, file it as an issue and I will treat it the same way.
—
Reply to this email directly, view it on GitHub <#215?email_source=notifications&email_token=CL5JVIKBMGDQQ5OPYOMJ5C35L5KBFA5CNFSNUABIM5UWIORPF5TWS5BNNB2WEL2ENFZWG5LTONUW63SDN5WW2ZLOOQXTCOBRGY3TSNRTUZZGKYLTN5XKO3LFNZ2GS33OUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussioncomment-18167963>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/CL5JVIKNDG44TWIKCXIHEKD5L5KBFAVCNFSNUABJKJSXA33TNF2G64TZHMYTCOJQGU2TONJUGU5UI2LTMN2XG43JN5XDWMJQGY2DENZQGSQXMAQ>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/CL5JVIMF7RFSSHXKPQUU7CT5L5KBFA5CNFSNUABIM5UWIORPF5TWS5BNNB2WEL2ENFZWG5LTONUW63SDN5WW2ZLOOQXTCOBRGY3TSNRTUZZGKYLTN5XKO3LFNZ2GS33OUVSXMZLOOSVGM33PORSXEX3JN5ZQ> and Android <https://github.com/notifications/mobile/android/CL5JVII5NXNZ4CEZ25F7WZD5L5KBFA5CNFSNUABIM5UWIORPF5TWS5BNNB2WEL2ENFZWG5LTONUW63SDN5WW2ZLOOQXTCOBRGY3TSNRTUZZGKYLTN5XKO3LFNZ2GS33OUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ>. Download it today!
You are receiving this because you were mentioned.
|
|
the curl | bash pattern and --dangerously-skip-permissions flag are both real behavioral signals worth taking seriously, not scanner noise. for the credential finding, check whether line 48 actually sends the token value in a request body/header vs just reading it to construct a local config object -- that distinction matters a lot. for the persistence finding in architecture.md, look at whether it describes a background process that auto-restarts on login without user confirmation. if the maintainer can't point you to the exact code path and explain why it's safe, treat the "DO NOT INSTALL" as the correct default, especially with client data in scope. (i work on 1claw, which is an agent secrets product, so i read a lot of these scans -- the TT3 env-var-to-network pattern is the one that most often turns out to be real rather than a false positive.) |
Uh oh!
There was an error while loading. Please reload this page.
Hi Eugeniu,
I'm evaluating obsidian-second-brain for my Second Brain vault and ran it through NVIDIA's SkillSpector (skillspector scan --no-llm) before installing, as a standard precaution for any third-party skill.
The scan returned a 100/100 CRITICAL score with a "DO NOT INSTALL" recommendation. The two most serious findings:
Plus 7 findings (category TT3) of credentials/API keys read from environment variables and sent in network calls.
I noticed a second scanner (Mondoo) flagged the same two core behaviors independently (the curl | bash install pattern and a background agent running with --dangerously-skip-permissions), while SkillsLLM's automated scan reported no high-severity issues.
Could you clarify what's actually happening in these two specific files/lines? I'd like to understand whether these are false positives (e.g. documented but unused code paths) or real behavior, before deciding whether to install this in a workflow that also touches client-related data.
Thanks for the work you've put into this project either way — happy to hear your side.
All reactions