Skip to content

improve(xtask): add mdbook check, fix directory redirects, and use safe signal handling#65

Open
SyedaAnshrahGillani wants to merge 2 commits intomicrosoft:mainfrom
SyedaAnshrahGillani:improve/xtask-server-and-signals
Open

improve(xtask): add mdbook check, fix directory redirects, and use safe signal handling#65
SyedaAnshrahGillani wants to merge 2 commits intomicrosoft:mainfrom
SyedaAnshrahGillani:improve/xtask-server-and-signals

Conversation

@SyedaAnshrahGillani
Copy link
Copy Markdown

This PR enhances the xtask automation tool by improving its reliability, safety, and correctness. These changes align with the "fail loudly" philosophy, ensuring the build and serve pipeline is robust and self-documenting.

Key Changes:

  1. Fail-Fast mdBook Validation:

    • Added a check_mdbook helper that verifies the presence of the mdbook binary in the system PATH.
    • Integrated this check into build and deploy commands to provide a clear, actionable error message early in the process, preventing late-stage execution failures.
  2. Robust Directory Resolution & Redirects:

    • Introduced a ResolveResult enum to strictly type the output of the file resolution logic (File, Redirect, NotFound).
    • Fixed a bug in the HTTP server where directory requests without a trailing slash (e.g., /python-book) would fail to
      resolve relative asset links.
    • The server now correctly issues a 301 Moved Permanently redirect to the slash-terminated path.
  3. Safe & Idiomatic Signal Handling:

    • Removed unsafe blocks and manual platform-specific extern "C" declarations for SIGINT (Ctrl+C).
    • Implemented signal handling using the ctrlc crate, ensuring a clean and safe exit across all operating systems.
  4. Dependency Management:

    • Updated xtask/Cargo.toml to include the ctrlc dependency.

These improvements ensure that the xtask tool—the entry point for building and previewing the training books—is stable,
predictable, and follows Rust best practices for CLI development.

@adarshdhital007
Copy link
Copy Markdown
Contributor

adarshdhital007 commented Mar 30, 2026

Thanks for the PR @SyedaAnshrahGillani. A few blockers before this can move forward:

  1. PR#18 overlap — This rewrites resolve_site_file, which I overhauled in PR#18 with multi-layer security (percent-decoding, .. traversal blocking, null byte checks, symlink escape prevention). Your changes seem to preserve these, but the description doesn't mention fix(docs, xtask): repair cross-book links and harden local serve #18 at all. Please explicitly confirm you've reviewed that PR and all security checks are intentionally kept intact.

  2. No testing notes — How was the 301 redirect behavior verified? A quick note would help reviewers trust the fix.

The ctrlc crate swap and ResolveResult enum are solid , just need the above sorted out.

@SyedaAnshrahGillani
Copy link
Copy Markdown
Author

Thanks for the review, @adarshdhital007!

  1. PR#18 Overlap: I have explicitly reviewed the security hardening from PR#18. I can confirm that all security checks (percent-decoding, traversal blocking, null byte checks, and symlink escape prevention) are intentionally preserved in the rewritten resolve_site_file. I've also added explicit comments to the code (in the latest commit) to document these checks and their origin.

  2. Testing Notes: The 301 redirect behavior and security checks were verified using a dedicated test script that simulates the server's resolution logic. The results confirmed:

    • 301 Redirects: Requesting a directory without a trailing slash (e.g., /python-book) correctly returns a 301 Moved Permanently redirect to /python-book/.
    • Security: Traversal attempts (..), null bytes, and symlink escapes are all correctly identified and blocked (returning NotFound).
    • Correct Resolution: Requests with trailing slashes correctly resolve to the index.html within the corresponding directory.

I've also verified that the new mdbook check correctly identifies its absence in the PATH, preventing late-stage build failures.

Let me know if you need any further clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants