Fix SST-715: urlencode returside i udskriv-redirect fra formfunk.php

[momocoder14]

What are the changes about?

returside blev indsat raa i redirect-URL til udskriv.php. Naar brugeren soeger paa kundenavn i ordrelisten indeholder returside-URL search- parametre som f.eks. &valg=faktura, der laekker ud som selvstaendige GET-parametre i udskriv.php. Dette medforer at $valg saettes til 'faktura' i stedet for 'pdf', PDF-fremviseren springes over, og brugeren ender paa en forkert 'Udskriftsvalg'-side.

Urlencode() paa $returside sikrer at & og ? i vaerdien er korrekt kodet og ikke fortolkes som URL-separatorer.

Have you checked the following?

• I have updated relevant documentation at https://github.com/DANOSOFT/saldi/wiki
• I have checked that i am not linking to any external resources such as external style- or JavaScript-files, that could compromise stability
• I have read and understood the developer guidelines
  https://docs.google.com/document/d/1GOmomtvKf21OV2VWNOIPweDi4gJHpMCK5qXzrPrypUc/edit?usp=sharing

Summary by CodeRabbit

• Bug Fixes
  • Improved form redirects by safely encoding return-page information, helping ensure users are sent back to the correct location after printing.
  • Restricted return-page links to relative paths and added safeguards for navigation links, preventing unsafe redirects and injected content.

[coderabbitai]

Warning

Review limit reached

@momocoder14, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 29f073b8-3897-4c34-bbc7-7ad47d9e1b8f

📥 Commits

Reviewing files that changed from the base of the PR and between 3929570 and 0952b2d.

📒 Files selected for processing (2)

• includes/formfunk.php
• includes/udskriv.php

📝 Walkthrough

Walkthrough

The change validates returside as a relative path and safely encodes or escapes it in print redirects, navigation links, and JavaScript redirects.

Changes

Return page security

Layer / File(s)	Summary
Validate return page paths includes/udskriv.php	returside now accepts empty values, ordreliste.php, or paths beginning with ../. Protocol and protocol-relative URLs are rejected. The changelog and file date are updated.
Encode return page usage includes/formfunk.php, includes/udskriv.php	Redirect query parameters and navigation links URL-encode returside. The JavaScript redirect uses JSON encoding, and the generated anchor uses HTML escaping.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: sirrolin

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the SST-715 fix and the URL-encoding change for the return URL in the redirect.
Description check	✅ Passed	The description explains the defect, the fix, and confirms all required checklist items.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[LuiHedlund02]

The urlencode fixes the reported &valg=faktura leakage, but two things
before merge:

1. $returside is untrusted GET input and includes/udskriv.php later
   emits it raw into JavaScript and href attributes — encoding it for this
   hop doesn't make the decoded value safe. Please restrict it to approved
   same-origin relative paths (and escape per output context where emitted).
2. When neither the caller nor GET supplies it, urlencode(null) on the
   $nomailantal > 0 path raises a PHP 8.1+ deprecation — cast
   (string)$returside or default to ''.

Retest with a return URL containing nested &/?, with no returside at
all, and with a hostile quote/JS payload.

[coderabbitai]

🔇 Additional comments (6)

includes/formfunk.php (1)

2265-2265: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

⚠️ Unverified finding
Sandbox verification was unavailable.

Normalize non-string returside values before encoding.

When a caller sends returside[]=..., $returside is an array. The explicit cast emits an Array to string conversion warning. An error handler that converts warnings to exceptions can stop the redirect before udskriv.php validates the value.

Accept only string values and use an empty fallback for other types.

Proposed fix

- urlencode((string)($returside ?? ''))
+ urlencode(is_string($returside ?? null) ? $returside : '')

Verify this behavior under the supported PHP version:

includes/udskriv.php (5)

7-7: LGTM!

Also applies to: 44-44

---

75-82: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

⚠️ Unverified finding
Sandbox verification was unavailable.

Reject malformed UTF-8 before context encoding.

The validator accepts byte sequences that are not valid UTF-8. For example, ../%FF can reach json_encode($returside), which can return false and render window.location.href = ;. The HTML output can also become an empty attribute value.

Reject invalid UTF-8 in the validator. Keep ENT_SUBSTITUTE as defense in depth for HTML output.

Suggested fix

 $returside = (function($s) {
     $s = trim((string)$s);
+    if (preg_match('//u', $s) !== 1) return '';
     if ($s === '' || $s === 'ordreliste.php') return $s;
...
- if ($returside) $href="\"" . htmlspecialchars($returside, ENT_QUOTES, 'UTF-8') . "\" accesskey=\"L\"";
+ if ($returside) $href="\"" . htmlspecialchars($returside, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . "\" accesskey=\"L\"";

Also applies to: 96-96, 465-465

---

75-82: 🗄️ Data Integrity & Integration

⚠️ Unverified finding
Sandbox verification was unavailable.

Verify the nav_back_url() output contract.

nav_back_url() returns a history entry directly and falls back to NAV_DEFAULT_URL in includes/stdFunc/navStack.php, Lines [72-83]. The new validator discards every value that does not start with ../. Confirm that all history entries and NAV_DEFAULT_URL use this form. Otherwise valid return navigation becomes empty.

---

408-408: LGTM!

---

434-434: LGTM!

Also applies to: 440-440, 446-446

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a8cc41e-aeb1-40b0-8c9d-56eb407cc812

📥 Commits

Reviewing files that changed from the base of the PR and between 5a0f648 and 3929570.

📒 Files selected for processing (2)

• includes/formfunk.php
• includes/udskriv.php