candy-shine: audit remediation steps 1-13#1164
Merged
Merged
Conversation
Mirrors charmbracelet/glamour parsing contract — malformed specs (ansi:300, ansi256:abc, etc.) now throw InvalidArgumentException instead of silently casting to 0 via blind (int) cast. Add theme.bad_color lang key.
…son path Add is_file() check in Theme::fromJson before reading — surfaces the common not-a-file/missing case cleanly rather than relying on the @-suppressed file_get_contents failure.
…moize textIsPlain Add stripControls() that removes C0 bytes except tab/newline — this closes the ANSI-injection vector for source-derived text. Constructor gains $sanitize=true flag (default on) and computes $textIsPlain once at construction (Step 11 memoization). Add withSanitize(bool) builder and sanitize() short alias. Apply sanitization to source literals in: renderText, renderCode (inline), renderIndent (formerly IndentedCode in renderNode), renderFencedCode (body before syntax highlighter), renderHtmlBlock, renderHtmlSpan. New SanitizeTest covers ESC-strip from text/inline-code/html-block/ fenced-code; sanitize=false passthrough; tab+newline preservation.
Add safeUrl() static method that strips C0/ESC/BEL from any URL and apply it unconditionally at resolveUrl() — the single choke point for all link/image hrefs and the visible (url) suffix. New SanitizeTest cases: hyperlink-url ESC/BEL stripped, image-url ESC/BEL stripped, normal URL unaffected.
Bare URL (autolink) now uses $theme->autolink style, falling back to $theme->link when autolink is null. New RendererTest cases: testAutolinkUsesAutolinkSlot (bold vs underline styling), testAutolinkFallsBackToLinkWhenAutolinkNotSet.
Replace hard-coded Border::rounded() with buildTableBorder() that constructs a Border using theme tableColumnSeparator/tableRowSeparator/ tableCenterSeparator glyphs while preserving rounded corners.
Register League\CommonMark\Extension\DescriptionList\DescriptionListExtension in the constructor. Add renderNode cases for DescriptionList/DescriptionTerm/ Description and implement renderDescriptionList/renderDescriptionTerm/ renderDescription with proper theme slot fallbacks. Note: DescriptionListExtension confirmed vendored in league/commonmark. New RendererTest cases: testDefinitionListRenders (styled with bold/italic slots), testDefinitionListWithNullStylesRenders (plain fallback).
conceal property was documented as a no-op placeholder with no readers. Remove the constructor param and doc-comment from Theme. Update RendererRound2Test::cloneWithOverrides slot list.
…rdering
Replace {see EmojiMap} with 'built-in shortcode map' in withEmoji
doc-comment since EmojiMap class does not exist.
Move the dangling @return list<int> doc-comment (was above expandEmojiShortcodes)
to its rightful position directly above extractBlankRuns method.
…t/margin Drop from BlockContext entirely — it was passed in on every block push but never read; availableWidth() is always computed on demand. Memoize accumulatedIndent and marginCount in BlockStack itself via running totals updated incrementally in push()/pop(). This changes O(n) traversal on every width query to O(1). Remove BlockStack::popTo() since it would corrupt the memoized counters — no caller in the renderer ever used it. Remove the two test methods that exercised it.
Add testApplyCaseUnknownFallsBackToIdentity: verifies that an unknown headingCase value in Theme falls through to identity (text unchanged). Add testWithSanitizeReturnsNewInstance: verifies that withSanitize(true) and withSanitize(false) each return a new distinct Renderer instance.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 2 medium |
| Security | 1 critical |
| CodeStyle | 4 minor |
🟢 Metrics 5 complexity · 0 duplication
Metric Results Complexity 5 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ansi:/ansi256:color specs with regex + range validation; addtheme.bad_colorlang keyBlockStackandStyleSheetfinal; addis_file()guard toTheme::fromJson()stripControls(),$sanitizeflag,$textIsPlainmemoization; wire into all text rendering methods; addwithSanitize()/sanitize()builders; addSanitizeTest.phpsafeUrl()stripping C0/ESC/BEL unconditionally inresolveUrl()$theme->autolink ?? $theme->linkbuildTableBorder()using theme slots with rounded-corner fallbacksDescriptionListExtension+renderNodecases forMdDescriptionList/DescriptionTerm/Descriptionconcealflag fromTheme{@see EmojiMap}doc-comment; fix stale doc-block placement atextractBlankRunsavailableWidthfromBlockContext; memoize indent/margin inBlockStack; remove unusedpopTo()methodtextIsPlainat construction (merged with Step 3)preg_match_all === falsecheckapplyCaseunknown case andwithSanitize()builderTest plan