refactor(admin-control-panel): extract shared awk script, fix div regex, add atomic write protection - #220
Conversation
…ex, add atomic write protection Agent-Logs-Url: https://github.com/EngineScript/EngineScript/sessions/c2a8fc75-8c5b-447a-8b1a-bb4b8a36fc1b Co-authored-by: PDowney <11467177+PDowney@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
…wk script with quoted heredoc Agent-Logs-Url: https://github.com/EngineScript/EngineScript/sessions/3eda393c-220d-4bba-a0d3-d19529a1bed9 Co-authored-by: PDowney <11467177+PDowney@users.noreply.github.com>
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the Admin Control Panel installer’s “remove Adminer tool card” logic by extracting duplicated awk depth-tracking into a shared awk program string, adjusts the <div> counting regex used for sanity-checking, and switches the index.html rewrite to a safer temp-file + move flow.
Changes:
- Extract duplicated awk depth-tracking logic into a single shared awk script (heredoc) and reuse it for both “extract” and “remove” modes.
- Update the
<div>open-tag counting regex used to validate the extracted block. - Add mktemp + trap cleanup and replace-in-place via
mvto reduce the risk of corruptingindex.html.
Comments suppressed due to low confidence (1)
scripts/install/tools/frontend/admin-control-panel-install.sh:85
- The NOTE block still refers to a “sed range” and claims the Adminer block must not contain nested
<div>elements, but the implementation now uses an awk depth counter specifically to handle nested<div>tags. Updating this comment to reflect the current awk-based approach (and its actual assumptions, e.g. the single-line opening tag grep pre-check) would prevent future confusion/mis-maintenance.
AWKEOF
# NOTE: This sed range depends on the HTML structure of index.html:
# - the Adminer card must be wrapped in a single <div ... id="adminer-tool" ...> ... </div> block
# - the opening <div> with id="adminer-tool" and its matching closing </div> must each be on a single line
# - the block must not contain nested <div> elements whose closing tags appear before the end of the card
| mv "$tmp_index" "${CONTROL_PANEL_INDEX}" | ||
| trap - EXIT INT TERM |
There was a problem hiding this comment.
mv "$tmp_index" "${CONTROL_PANEL_INDEX}" is not checked for success, but the cleanup trap is cleared unconditionally right after. If mv fails, the script will silently continue with the original index.html and may leak the temp file because the trap was removed. Consider checking mv's exit status (and only clearing the trap on success), and exiting with an error if the move fails so the install doesn't proceed in a partially-updated state.
| mv "$tmp_index" "${CONTROL_PANEL_INDEX}" | |
| trap - EXIT INT TERM | |
| if mv "$tmp_index" "${CONTROL_PANEL_INDEX}"; then | |
| trap - EXIT INT TERM | |
| else | |
| echo "Error: Failed to replace index.html after Adminer card removal." >&2 | |
| exit 1 | |
| fi |
AWK_ADMINER_BLOCK_SCRIPT='...'with a quoted heredoc (read -r -d '' AWK_ADMINER_BLOCK_SCRIPT << 'AWKEOF' || true) to resolve shellcheck SC2016 without requiring backslash-escaping every$and"in the awk programOriginal prompt