-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Remove inappropriate usermod environment changes #5720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
9e10f31
a1fb347
4ab8de3
ef39d68
78df59c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,41 +22,51 @@ jobs: | |
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| - name: Get changed usermod environments | ||
| - name: Get changed usermod build matrix | ||
| id: envs | ||
| run: | | ||
| # Usermods whose directories changed in this PR | ||
| changed=$(git diff --name-only ${{ github.event.pull_request.base.sha }} HEAD \ | ||
| | grep '^usermods/' | cut -d/ -f2 | sort -u || true) | ||
|
|
||
| # All usermods with a library.json (excluding known-incompatible ones) | ||
| all=$(find usermods/ -name library.json \ | ||
| | xargs dirname | xargs -n 1 basename \ | ||
| | grep -v PWM_fan | grep -v BME68X_v2 | grep -v pixels_dice_tray \ | ||
| | sort || true) | ||
| # Shared override defining the standard build environments | ||
| default_ini="usermods/platformio_override.usermods.ini" | ||
|
|
||
| if [ -z "$changed" ] || [ -z "$all" ]; then | ||
| echo "usermods=[]" >> $GITHUB_OUTPUT | ||
| else | ||
| usermods=$(comm -12 <(echo "$all") <(echo "$changed") | jq -R | jq --slurp -c) | ||
| echo "usermods=$usermods" >> $GITHUB_OUTPUT | ||
| fi | ||
| matrix='[]' | ||
| for mod in $changed; do | ||
| # Skip usermods known to be incompatible | ||
| case "$mod" in PWM_fan|BME68X_v2|pixels_dice_tray) continue ;; esac | ||
| # Only build usermods that ship a library.json | ||
| [ -f "usermods/$mod/library.json" ] || continue | ||
|
|
||
| # A usermod may provide its own build environments via a sample override; | ||
| # otherwise the standard environments from the shared override are used. | ||
| # Either way, the env list comes from the ini file rather than being | ||
| # duplicated here. | ||
| sample="usermods/$mod/platformio_override.ini.sample" | ||
| [ -f "$sample" ] && ini="$sample" || ini="$default_ini" | ||
| envs=$(grep -oE '^\[env:[^]]+\]' "$ini" | sed 's/^\[env:\(.*\)\]$/\1/') | ||
|
|
||
| for env in $envs; do | ||
| matrix=$(echo "$matrix" | jq --arg u "$mod" --arg e "$env" -c '. + [{usermod: $u, env: $e}]') | ||
| done | ||
| done | ||
| echo "matrix=$matrix" >> $GITHUB_OUTPUT | ||
| outputs: | ||
| usermods: ${{ steps.envs.outputs.usermods }} | ||
| matrix: ${{ steps.envs.outputs.matrix }} | ||
|
|
||
|
|
||
| build: | ||
| # Only run for pull requests from forks (not from branches within wled/WLED) | ||
| # Skip when no changed usermods were found (e.g. only non-library changes) | ||
| if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository && needs.get_usermod_envs.outputs.usermods != '[]' | ||
| name: Build Enviornments | ||
| # Skip when no buildable usermods were found (e.g. only non-library changes) | ||
| if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository && needs.get_usermod_envs.outputs.matrix != '[]' | ||
| name: Build (${{ matrix.usermod }} / ${{ matrix.env }}) | ||
| runs-on: ubuntu-latest | ||
| needs: get_usermod_envs | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| usermod: ${{ fromJSON(needs.get_usermod_envs.outputs.usermods) }} | ||
| environment: [usermods_esp32, usermods_esp32c3, usermods_esp32s2, usermods_esp32s3] | ||
| include: ${{ fromJSON(needs.get_usermod_envs.outputs.matrix) }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Set up Node.js | ||
|
|
@@ -72,102 +82,28 @@ jobs: | |
| ~/.platformio/.cache | ||
| ~/.buildcache | ||
| build_output | ||
| key: pio-${{ runner.os }}-${{ matrix.environment }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}-${{ hashFiles('wled00/**', 'usermods/**') }} | ||
| restore-keys: pio-${{ runner.os }}-${{ matrix.environment }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}- | ||
| key: pio-${{ runner.os }}-${{ matrix.env }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}-${{ hashFiles('wled00/**', 'usermods/**') }} | ||
| restore-keys: pio-${{ runner.os }}-${{ matrix.env }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}- | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' | ||
| cache: 'pip' | ||
| - name: Install PlatformIO | ||
| run: pip install -r requirements.txt | ||
| - name: Add usermods environment | ||
| - name: Configure build environment | ||
| run: | | ||
| cp -v usermods/platformio_override.usermods.ini platformio_override.ini | ||
| echo >> platformio_override.ini | ||
| echo "custom_usermods = ${{ matrix.usermod }}" >> platformio_override.ini | ||
| cat platformio_override.ini | ||
|
|
||
| - name: Build firmware | ||
| run: pio run -e ${{ matrix.environment }} | ||
|
|
||
|
|
||
| get_custom_build_envs: | ||
| name: Gather Custom Build Environments | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| - name: Find usermods with custom build environments | ||
| id: custom_envs | ||
| run: | | ||
| # On PRs: only scan usermods whose directories changed. | ||
| # On push: scan all usermods (validates the full set on merge). | ||
| if [ "${{ github.event_name }}" = "pull_request" ]; then | ||
| changed=$(git diff --name-only ${{ github.event.pull_request.base.sha }} HEAD \ | ||
| | grep '^usermods/' | cut -d/ -f2 | sort -u || true) | ||
| if [ -z "$changed" ]; then | ||
| echo "matrix=[]" >> $GITHUB_OUTPUT | ||
| exit 0 | ||
| fi | ||
| samples=$(for mod in $changed; do | ||
| f="usermods/$mod/platformio_override.ini.sample" | ||
| [ -f "$f" ] && echo "$f" | ||
| done | sort) | ||
| # Use the usermod's own override when it provides one, otherwise apply | ||
| # the shared usermods override with this usermod enabled. | ||
| sample="usermods/${{ matrix.usermod }}/platformio_override.ini.sample" | ||
| if [ -f "$sample" ]; then | ||
| cp -v "$sample" platformio_override.ini | ||
| else | ||
| samples=$(find usermods/ -name "platformio_override.ini.sample" | sort) | ||
| cp -v usermods/platformio_override.usermods.ini platformio_override.ini | ||
| echo >> platformio_override.ini | ||
| echo "custom_usermods = ${{ matrix.usermod }}" >> platformio_override.ini | ||
| fi | ||
| cat platformio_override.ini | ||
|
|
||
| result='[]' | ||
| for sample in $samples; do | ||
| usermod=$(dirname "$sample" | xargs basename) | ||
| # Skip usermods known to be incompatible (same list as get_usermod_envs) | ||
| case "$usermod" in PWM_fan|BME68X_v2|pixels_dice_tray) continue ;; esac | ||
| envs=$(grep -E '^\[env:[^]]+\]' "$sample" | sed 's/^\[env:\(.*\)\]$/\1/') | ||
| for env in $envs; do | ||
| result=$(echo "$result" | jq --arg u "$usermod" --arg e "$env" '. + [{usermod: $u, env: $e}]') | ||
| done | ||
| done | ||
| echo "matrix=$(echo "$result" | jq -c '.')" >> $GITHUB_OUTPUT | ||
| outputs: | ||
| matrix: ${{ steps.custom_envs.outputs.matrix }} | ||
|
|
||
|
|
||
| build_custom: | ||
| name: Build Custom Env (${{ matrix.usermod }} / ${{ matrix.env }}) | ||
| runs-on: ubuntu-latest | ||
| needs: get_custom_build_envs | ||
| if: needs.get_custom_build_envs.outputs.matrix != '[]' | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| include: ${{ fromJSON(needs.get_custom_build_envs.outputs.matrix) }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| cache: 'npm' | ||
| - run: npm ci | ||
| - name: Cache PlatformIO | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.platformio/.cache | ||
| ~/.buildcache | ||
| build_output | ||
| key: pio-${{ runner.os }}-${{ matrix.env }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}-${{ hashFiles('wled00/**', 'usermods/**') }} | ||
| restore-keys: pio-${{ runner.os }}-${{ matrix.env }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}- | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' | ||
| cache: 'pip' | ||
| - name: Install PlatformIO | ||
| run: pip install -r requirements.txt | ||
| - name: Apply custom build environment | ||
| run: cp -v "usermods/${{ matrix.usermod }}/platformio_override.ini.sample" platformio_override.ini | ||
| - name: Build firmware | ||
| run: pio run -e ${{ matrix.env }} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: set -euo pipefail
wc -l .github/workflows/usermods.yml
cat -n .github/workflows/usermods.yml | sed -n '1,180p'Repository: wled/WLED Length of output: 5058 🏁 Script executed: set -euo pipefail
grep -nE '^(on:|permissions:|- name:|run:|matrix:|grep -oE|custom_usermods|pio run -e)' .github/workflows/usermods.ymlRepository: wled/WLED Length of output: 153 🏁 Script executed: set -euo pipefail
wc -l .github/workflows/usermods.yml
cat -n .github/workflows/usermods.yml | sed -n '1,180p'
grep -nE '^(on:|permissions:|- name:|run:|matrix:|grep -oE|custom_usermods|pio run -e)' .github/workflows/usermods.ymlRepository: wled/WLED Length of output: 5064 Quote the matrix values before using them in shell
🤖 Prompt for AI AgentsSource: Path instructions
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this new to the PR, or was the previous logic also vulnerable?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
gh pr view 5720 --repo wled/WLED --json baseRefOid,headRefOid,baseRefName,headRefName
gh api repos/wled/WLED/contents/.github/workflows/usermods.yml?ref=$(gh pr view 5720 --repo wled/WLED --json baseRefOid --jq .baseRefOid) --jq .content | base64 -d > usermods_base.yml
grep -nE 'matrix\.(usermod|env|environment)' usermods_base.ymlLength of output: 1655
So this isn't new to your PR — the previous logic (both the That said, worth noting the practical exposure is bounded here: this workflow only runs for Since this predates your PR, I won't hold up this PR on it — happy to open a separate tracking issue if you'd like. 🧠 Learnings used
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's still worth to check if the vulnerability gap can be closed with a few changes. Even when it predates this PR (so no regression).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No objections from me! The changes seem reasonably achievable. I just wanted to be sure it wasn't something I'd done inadvertently - I'm not an expert in Github Actions - and I would like to move quickly on this PR as I consider it important. FWIW, CodeRabbit has been doing this to me a lot lately. I've been using to check some of my in-progress work before opening a PR, and it keeps finding bugs like this around areas I've been working that have always been there but could really use attention. Makes for better code, but it sure seems like a drag on getting things done... ;)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I was hoping that CodeRabbit would start to highlight such improvement opportunities with all the rules I've added to its database. Especially for code robustness and "cyber security", I think we've collected a lot of technical debt that might "fall onto our feet" one day. I think it's good to spend more time if that makes our codebase better - even at the price of "reducing feature delivery speed" (not sure if my wording makes sense in english ;) ). I've experienced this in my day job, too: "clean up the module while you work on it" does pay back in terms of quality, even when it's sometimes conflicting with the intuition of "don't fix stuff that's not (proven to be) broken". |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -714,7 +714,7 @@ build_flags = ${common.build_flags} ${esp32s2.build_flags} -D WLED_RELEASE_NAME= | |
| [env:usermods] | ||
| extends = env:esp32dev | ||
| build_flags = ${common.build_flags} ${esp32_idf_V4.build_flags} -D WLED_RELEASE_NAME=\"ESP32_USERMODS\" | ||
| -DTOUCH_CS=9 -DWLED_USE_SD_SPI ;; help a few usermods that require special flags to compile | ||
| -DTOUCH_CS=9 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== platformio.ini line count =="
wc -l platformio.ini
echo
echo "== around referenced line 717 =="
nl -ba platformio.ini | sed -n '690,740p'
echo
echo "== all TOUCH_CS occurrences =="
rg -n "TOUCH_CS|custom_usermods|\\[env:usermods\\]|\\[env:" platformio.ini
echo
echo "== env headers around usermods =="
python3 - <<'PY'
from pathlib import Path
text = Path('platformio.ini').read_text().splitlines()
for i, line in enumerate(text, 1):
if line.strip().startswith('[env:') or 'TOUCH_CS' in line or 'custom_usermods' in line:
start = max(1, i-3)
end = min(len(text), i+3)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4d}: {text[j-1]}")
PYRepository: wled/WLED Length of output: 272 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== platformio.ini context around TOUCH_CS =="
nl -ba platformio.ini | sed -n '700,730p'
echo
echo "== all env headers and custom_usermods references =="
rg -n "^\[env:|custom_usermods|TOUCH_CS" platformio.ini
echo
echo "== nearby override envs if present =="
python3 - <<'PY'
from pathlib import Path
lines = Path('platformio.ini').read_text().splitlines()
for i, line in enumerate(lines, 1):
if line.strip().startswith('[env:') and ('usermods' in line.lower() or 'override' in line.lower() or 'touch' in line.lower()):
start = max(1, i-2)
end = min(len(lines), i+18)
print(f"\n--- {i}: {line} ---")
for j in range(start, end+1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: wled/WLED Length of output: 232 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== lines 700-730 with line numbers =="
awk 'NR>=700 && NR<=730 { printf "%4d: %s\n", NR, $0 }' platformio.ini
echo
echo "== all occurrences of TOUCH_CS, custom_usermods, and relevant env headers =="
python3 - <<'PY'
from pathlib import Path
lines = Path('platformio.ini').read_text().splitlines()
for i, line in enumerate(lines, 1):
if 'TOUCH_CS' in line or 'custom_usermods' in line or line.strip().startswith('[env:'):
print(f"{i:4d}: {line}")
PY
echo
echo "== locate [env:usermods] block =="
python3 - <<'PY'
from pathlib import Path
lines = Path('platformio.ini').read_text().splitlines()
start = None
for i, line in enumerate(lines, 1):
if line.strip() == '[env:usermods]':
start = i
break
if start is None:
print("missing [env:usermods]")
raise SystemExit(0)
end = len(lines)
for j in range(start + 1, len(lines) + 1):
if lines[j-1].startswith('[env:') and j != start:
end = j - 1
break
for j in range(start, min(end, start + 40) + 1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: wled/WLED Length of output: 5366 Keep 🤖 Prompt for AI AgentsSource: Path instructions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @willmmiles
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "TOUCH_CS" --type=cpp --type=ini -g '*.ini' -g '*.h' -g '*.cpp' . || true
rg -n "TOUCH_CS" .Length of output: 331
Given that, and consistent with this PR's stated goal of keeping the shared
I'll leave the final call to you/willmmiles since you have more context on which usermods currently rely on this workaround in CI. 🧠 Learnings used
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's actually necessary. I'm almost done the PR for the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| custom_usermods = * ; Expands to all usermods in usermods folder | ||
| board_build.partitions = ${esp32.extreme_partitions} ; We're gonna need a bigger boat | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.