Skip to content

Commit 81ddf9b

Browse files
authored
Only allow listing when list verb exists (#1026)
1 parent d95e703 commit 81ddf9b

File tree

2 files changed

+95
-0
lines changed

2 files changed

+95
-0
lines changed

pkg/stores/sqlproxy/proxy_store.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1122,6 +1122,9 @@ func (s *Store) cacheForWithDeps(ctx context.Context, apiOp *types.APIRequest, a
11221122
}
11231123

11241124
func (s *Store) cacheFor(ctx context.Context, apiOp *types.APIRequest, apiSchema *types.APISchema) (*factory.Cache, error) {
1125+
if !canList(apiSchema) {
1126+
return nil, apierror.NewAPIError(validation.MethodNotAllowed, fmt.Sprintf("resource %s is not a listable resource", apiSchema.ID))
1127+
}
11251128
// warnings from inside the informer are discarded
11261129
buffer := WarningBuffer{}
11271130
client, err := s.clientGetter.TableAdminClient(apiOp, apiSchema, "", &buffer)
@@ -1144,3 +1147,13 @@ func (s *Store) cacheFor(ctx context.Context, apiOp *types.APIRequest, apiSchema
11441147
}
11451148
return inf, nil
11461149
}
1150+
1151+
func canList(schema *types.APISchema) bool {
1152+
for _, verb := range attributes.Verbs(schema) {
1153+
if verb == "list" {
1154+
return true
1155+
}
1156+
}
1157+
1158+
return false
1159+
}
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package tests
2+
3+
import (
4+
"database/sql"
5+
"fmt"
6+
"net/http"
7+
"net/http/httptest"
8+
"time"
9+
10+
"github.com/rancher/steve/pkg/server"
11+
"github.com/rancher/steve/pkg/sqlcache/informer/factory"
12+
"github.com/stretchr/testify/assert"
13+
"k8s.io/apimachinery/pkg/runtime/schema"
14+
_ "modernc.org/sqlite"
15+
)
16+
17+
func (i *IntegrationSuite) TestIssue51930() {
18+
ctx := i.T().Context()
19+
// Start steve with SQL Cache enabled
20+
steveHandler, err := server.New(ctx, i.restCfg, &server.Options{
21+
SQLCache: true,
22+
SQLCacheFactoryOptions: factory.CacheFactoryOptions{
23+
GCInterval: 15 * time.Minute,
24+
GCKeepCount: 1000,
25+
},
26+
})
27+
i.Require().NoError(err)
28+
29+
httpServer := httptest.NewServer(steveHandler)
30+
defer httpServer.Close()
31+
32+
baseURL := httpServer.URL
33+
34+
// Wait for schemas to be ready
35+
// We wait for TokenReviews schema to be registered in Steve.
36+
// This confirms Steve has completed discovery.
37+
i.waitForSchema(baseURL, schema.GroupVersionResource{Group: "authentication.k8s.io", Version: "v1", Resource: "tokenreviews"})
38+
39+
// The TokenReview schema ID.
40+
schemaID := "authentication.k8s.io.tokenreviews"
41+
42+
// Attempt to list TokenReviews
43+
url := fmt.Sprintf("%s/v1/%s", baseURL, schemaID)
44+
45+
client := &http.Client{
46+
Timeout: 20 * time.Second,
47+
}
48+
49+
fmt.Printf("Making request to %s\n", url)
50+
resp, err := client.Get(url)
51+
52+
// If the request times out, err will be a timeout error.
53+
// We expect the request to return (likely with an error status because TokenReview is not listable).
54+
// But crucially, we expect it NOT to hang.
55+
i.Require().NoError(err, "Request timed out or failed")
56+
defer resp.Body.Close()
57+
58+
fmt.Printf("TokenReview list response: %d\n", resp.StatusCode)
59+
60+
// Check if the table was created in the SQLite database.
61+
// The DB path is in i.sqliteDatabaseFile
62+
63+
db, err := sql.Open("sqlite", i.sqliteDatabaseFile)
64+
i.Require().NoError(err)
65+
defer db.Close()
66+
67+
// The table name usually follows the GVK pattern: group_version_kind
68+
// authentication.k8s.io_v1_TokenReview
69+
tableName := "authentication.k8s.io_v1_TokenReview"
70+
71+
var name string
72+
err = db.QueryRow("SELECT name FROM sqlite_master WHERE type='table' AND name=?", tableName).Scan(&name)
73+
74+
// We expect this to Fail (err == sql.ErrNoRows) if the bug is fixed.
75+
// If the bug is present, the table might have been created (if the request didn't hang before creating it).
76+
// Actually, the hang usually happens *because* it tries to start an informer for a non-watchable resource.
77+
// If it starts the informer, it likely creates the table first.
78+
// So checking for table existence is a good proxy for "did it try to cache this?".
79+
80+
// If the fix is to PREVENT starting the cache, then the table should not exist.
81+
assert.ErrorIs(i.T(), err, sql.ErrNoRows, "Table %s should not exist", tableName)
82+
}

0 commit comments

Comments
 (0)