kunwp1 commented on code in PR #5138:
URL: https://github.com/apache/texera/pull/5138#discussion_r3293809369
##########
access-control-service/src/main/scala/org/apache/texera/service/resource/AccessControlResource.scala:
##########
@@ -60,7 +64,8 @@ object AccessControlResource extends LazyLogging {
logger.info(s"Authorizing request for path: $path")
path match {
- case wsapiWorkflowWebsocket() | apiExecutionsStats() |
apiExecutionsResultExport() =>
+ case wsapiWorkflowWebsocket() | apiExecutionsStats() |
apiExecutionsResultExport() |
+ pveRoute() =>
Review Comment:
PR description says "Tested manually." Worth adding a small unit test on
`AccessControlResource.authorize` that covers such as:
- `/pve/system?cuid=N` → 200 (query-string cuid)
- `/pve/pves/N` → 200 (path-segment cuid via the DELETE route)
- `/pve/N/myenv/packages/numpy` → 200 (path-segment cuid via the packages
route)
- `/pve/no-cuid-anywhere` → 403 (cuid extraction falls through to empty →
NumberFormatException → FORBIDDEN)
- a non-PVE garbage path → 403
##########
access-control-service/src/main/scala/org/apache/texera/service/resource/AccessControlResource.scala:
##########
@@ -43,6 +43,10 @@ object AccessControlResource extends LazyLogging {
private val wsapiWorkflowWebsocket: Regex =
""".*/wsapi/workflow-websocket.*""".r
private val apiExecutionsStats: Regex =
""".*/api/executions/[0-9]+/stats/[0-9]+.*""".r
private val apiExecutionsResultExport: Regex =
""".*/api/executions/result/export.*""".r
+ private val pveRoute: Regex = """.*/(?:api/|wsapi/)?pve(?:/.*)?""".r
Review Comment:
Suggested by Claude:
`.*/(?:api/|wsapi/)?pve(?:/.*)?` is overly permissive — the leading `.*/`
will match any path ending in `…/pve` or `…/pve/anything`, not just the
expected `/api/pve` / `/wsapi/pve` / `/pve` shapes. Consistent with how
`wsapiWorkflowWebsocket` / `apiExecutionsStats` are written above, so not out
of line for this file, but the PVE routes here are well-defined enough to
anchor more tightly, e.g.:
```scala
private val pveRoute: Regex =
"""^/?(?:auth/)?(?:api/|wsapi/)?pve(?:/.*)?$""".r
```
Also applies to `pvePvesCuidPath` and `pvePackagesCuidPath` below. Worth
double-checking whether `uriInfo.getPath` here includes the `auth/` prefix from
the enclosing `@Path("/auth")` resource — your manual test probably already
covered this, but the regex shape depends on it.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala:
##########
@@ -36,12 +36,10 @@ class PveResource {
@GET
@Path("/system")
@Produces(Array(MediaType.APPLICATION_JSON))
- def getSystemPackages: util.Map[String, util.List[String]] = {
+ def getSystemPackages(
+ @QueryParam("isLocal") isLocal: Boolean
Review Comment:
`isLocal` describes where the backend is running. I can see that there is a
security issue where a malicious user can flip `isLocal`. I suggest to derive
`isLocal` from `KubernetesConfig` (there's already a config flag for this) and
drop the param.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
