-
-
Notifications
You must be signed in to change notification settings - Fork 302
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a no_listing configuration option to prevent directory indexing over HTTP, addressing issue #53 by making directory paths return 404 while still allowing direct file access.
Changes:
- Added
no_listingto server configuration and documented it in example config/docs. - Implemented directory-listing blocking in the local filesystem (
pkg/fs.Local) by returningos.ErrNotExistfor directories when enabled. - Updated and added tests to cover default vs enabled behavior and adjusted
NewLocalcall sites for the new parameter.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
services/web/server.go |
Adds NoListing to the server config struct for TOML/config plumbing. |
services/web/server_test.go |
Adds HTTP-level tests validating directory listing vs 404 behavior using local storage. |
pkg/fs/local.go |
Implements the actual "no directory listing" behavior in Local.Open. |
pkg/fs/local_test.go |
Adds unit tests for Local.Open behavior with NoListing enabled/disabled. |
cmd/podsync/main.go |
Wires cfg.Server.NoListing into fs.NewLocal(...). |
services/migrate/migrate_test.go |
Updates fs.NewLocal(...) calls for the new function signature. |
config.toml.example |
Documents no_index and no_listing in the example configuration. |
CLAUDE.md |
Updates configuration documentation to include no_listing. |
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cfg := Config{ | ||
| Port: 8080, | ||
| Path: "", | ||
| NoListing: true, | ||
| } |
Copilot
AI
Mar 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test, cfg.NoListing doesn't affect behavior because web.New never reads Config.NoListing (directory listing is controlled by the http.FileSystem implementation passed in). To avoid confusion, consider removing NoListing from cfg here and/or renaming the test to reflect that it's validating the storage-backed behavior rather than server config plumbing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude can you make a follow up PR to address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude finished @mxpv's task in 1m 45s ---- View job * claude/pr-845-20260309-0339 * Create PR
Follow-up: Fix misleading NoListing in server test
- Gather context
- Read the test file to understand current state
- Fix the test (remove
NoListingfrom cfg and add clarifying comment) - Run tests to verify
- Commit and push changes
- Create PR
Change made: Removed NoListing: true from the cfg struct in TestNoListingEnabledWhenConfigured (services/web/server_test.go:162) since web.New never reads Config.NoListing. Directory listing behavior is controlled by the http.FileSystem implementation (the Local storage passed in), not server config. Added a comment in the test to clarify this.