From 4df78d13513de65c2638a477b5a96276afa30708 Mon Sep 17 00:00:00 2001
From: Gavin <gaprice@lbl.gov>
Date: Mon, 6 May 2024 17:17:23 -0700
Subject: [PATCH 1/5] Remove build cruft

Obsolete with switch to GHA
---
 build/build_docker_image.sh |  7 -------
 build/push2dockerhub.sh     | 32 --------------------------------
 2 files changed, 39 deletions(-)
 delete mode 100755 build/build_docker_image.sh
 delete mode 100755 build/push2dockerhub.sh

diff --git a/build/build_docker_image.sh b/build/build_docker_image.sh
deleted file mode 100755
index cbe7dd2..0000000
--- a/build/build_docker_image.sh
+++ /dev/null
@@ -1,7 +0,0 @@
-export BRANCH=${TRAVIS_BRANCH:-`git symbolic-ref --short HEAD`}
-export DATE=`date -u +"%Y-%m-%dT%H:%M:%SZ"`
-export COMMIT=${TRAVIS_COMMIT:-`git rev-parse --short HEAD`}
-docker build --build-arg BUILD_DATE=$DATE \
-             --build-arg VCS_REF=$COMMIT \
-             --build-arg BRANCH=$BRANCH \
-             -t kbase/blobstore:$COMMIT .
\ No newline at end of file
diff --git a/build/push2dockerhub.sh b/build/push2dockerhub.sh
deleted file mode 100755
index f8fe1ab..0000000
--- a/build/push2dockerhub.sh
+++ /dev/null
@@ -1,32 +0,0 @@
-#!/bin/bash
-# 
-# This script is intended to be run in the deploy stage of a travis build
-# It checks to make sure that this is a not a PR, and that we have the secure
-# environment variables available and then checks if this is either the master
-# or develop branch, otherwise we don't push anything
-#
-# NOTE: IMAGE_NAME is expected to be passed in via the environment so that this
-# script can be more general
-#
-# sychan@lbl.gov
-# 8/31/2017
-
-TAG=`if [ "$TRAVIS_BRANCH" == "master" ]; then echo "latest"; else echo $TRAVIS_BRANCH ; fi`
-COMMIT=${TRAVIS_COMMIT:-`git rev-parse --short HEAD`}
-
-if ( [ "$TRAVIS_SECURE_ENV_VARS" == "true" ] && [ "$TRAVIS_PULL_REQUEST" == "false" ] ); then
-    # $TAG was set from TRAVIS_BRANCH, which is a little wonky on pull requests,
-    # but it should be okay since we should never get here on a PR
-    if  ( [ "$TAG" == "latest" ] || [ "$TAG" == "develop" ] ) ; then
-        echo "Logging into Dockerhub as $DOCKER_USER"
-        docker login -u $DOCKER_USER -p $DOCKER_PASS && \
-        docker tag $IMAGE_NAME:$COMMIT $IMAGE_NAME:$TAG && \
-        echo "Pushing $IMAGE_NAME:$TAG" && \
-        docker push $IMAGE_NAME:$TAG || \
-        ( echo "Failed to login and push tagged image" && exit 1 )
-    else
-        echo "Not pushing image for branch $TAG"
-    fi
-else
-    echo "Not building image for pull requests or if secure variables unavailable"
-fi
\ No newline at end of file

From 790928ab25a0b141e80eaadfb2e3b0a669e57265 Mon Sep 17 00:00:00 2001
From: Gavin <gaprice@lbl.gov>
Date: Mon, 6 May 2024 17:23:56 -0700
Subject: [PATCH 2/5] Fix tset name & add codecov token

---
 .github/workflows/test.yml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 9a806ff..eb8d8b3 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -18,7 +18,7 @@ env:
   AUTH2_JAR_NAME: kbase-auth2-test-shadow-all-0.7.0.jar
 
 jobs:
-  workspace_deluxe_tests:
+  blobstore_tests:
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
@@ -84,4 +84,5 @@ jobs:
       - name: Upload coverage to Codecov
         uses: codecov/codecov-action@v3
         with:
+          token: ${{ secrets.CODECOV_TOKEN }}
           fail_ci_if_error: true

From 44bf972da6eb89a8725e01fe7f7f33057fbf3182 Mon Sep 17 00:00:00 2001
From: Gavin <gaprice@lbl.gov>
Date: Wed, 8 May 2024 19:32:39 -0700
Subject: [PATCH 3/5] Add query param to delete after file stream

This is useful for temporary nodes where once the user has the data, the node is no longer needed, and the link to the data is expected to work once.
---
 README.md                   |  6 +++-
 RELEASE_NOTES.md            | 17 +++++----
 service/errortypes.go       | 16 +++++++++
 service/integration_test.go | 72 ++++++++++++++++++++++++++++++++++++-
 service/server.go           | 31 ++++++++++++++++
 5 files changed, 134 insertions(+), 8 deletions(-)

diff --git a/README.md b/README.md
index 8033038..cebf585 100644
--- a/README.md
+++ b/README.md
@@ -171,7 +171,7 @@ RETURNS: an ACL.
 ## Download a file from a node
 ```
 AUTHORIZATION OPTIONAL
-GET /node/<id>?download[_raw][&seek=#][&length=#]
+GET /node/<id>?download[_raw][&seek=#][&length=#][&del]
 
 RETURNS: the file content.
 ```
@@ -186,6 +186,10 @@ to the file size is an error. Defaults to 0.
 `length` may be greater than the remaining file length. Defaults to 0, which indicates that the
 remainder of the file should be returned.
 
+`del` causes the node to be deleted once the file contents have been streamed. The user must
+be the node owner or a service administrator. Note this is playing very fast and loose with the
+semantics of an HTTP GET.
+
 ## Set a node to be publicly readable
 ```
 AUTHORIZATION REQUIRED
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 43c2f8c..fb7efea 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,17 +1,22 @@
+# 0.1.4
+
+* Added the `del` param when downloading the file from a node.
+
 # 0.1.3
 
-- Added GHA workflows and removed Travis CI
-- MongoController is now compatible with Mongo versions 2 through 7
-- Updated test config file to specify the auth2 shadow jar path vs. the jars repo path
+* Added GHA workflows and removed Travis CI
+* MongoController is now compatible with Mongo versions 2 through 7
+* Updated test config file to specify the auth2 shadow jar path vs. the jars repo path
 
 # 0.1.2
 
-- Support for disabling SSL verification of remote S3 certificates (default false) with the s3-disable-ssl-verify option in the configuration file.
+* Support for disabling SSL verification of remote S3 certificates (default false)
+  with the s3-disable-ssl-verify option in the configuration file.
 
 # 0.1.1
 
-- Added seek & length parameters to file download requests
+* Added seek & length parameters to file download requests
 
 # 0.1.0
 
-- Initial release
+* Initial release
diff --git a/service/errortypes.go b/service/errortypes.go
index b08e72e..8c7ebe4 100644
--- a/service/errortypes.go
+++ b/service/errortypes.go
@@ -14,6 +14,20 @@ const (
 	invalidAuthHeader = "Invalid authorization header or content"
 )
 
+// UnauthorizedCustomError denotes that an unauthorized operation was requested that needs
+// special explanation in the error string.
+type UnauthorizedCustomError string
+
+// UnauthorizedCustomError creates a new UnauthorizedCustomError.
+func NewUnauthorizedCustomError(err string) *UnauthorizedCustomError {
+	e := UnauthorizedCustomError(err)
+	return &e
+}
+
+func (e *UnauthorizedCustomError) Error() string {
+	return string(*e)
+}
+
 func translateError(err error) (code int, errstr string) {
 	// not sure about this approach. Alternative is to add some state to every error that
 	// can be mapped to a code, and I'm not super thrilled about that either.
@@ -30,6 +44,8 @@ func translateError(err error) (code int, errstr string) {
 		// Shock compatibility, really should be 403 forbidden
 		return http.StatusBadRequest, "Users that are not node owners can only delete " +
 			"themselves from ACLs."
+	case *UnauthorizedCustomError:
+		return http.StatusUnauthorized, t.Error()
 	case *auth.InvalidUserError:
 		// no equivalent shock error, it accepts any string as a username
 		return http.StatusBadRequest, t.Error()
diff --git a/service/integration_test.go b/service/integration_test.go
index ae4a07b..4877f94 100644
--- a/service/integration_test.go
+++ b/service/integration_test.go
@@ -544,6 +544,23 @@ func (t *TestSuite) TestStoreAndGetNodeAsAdminWithFormatAndTrailingSlashAndSeekA
 	t.checkFile(t.url+path+dlrlen, path, &t.kBaseAdmin, 7, "", []byte("foobarb"))
 }
 
+func (t *TestSuite) TestGetFileWithDelete() {
+	t.testGetFileWithDelete(t.noRole)
+	t.testGetFileWithDelete(t.stdRole)
+}
+
+func (t *TestSuite) testGetFileWithDelete(deleter User) {
+	body := t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
+		"OAuth "+t.noRole.token, 374, 200)
+	id := (body["data"].(map[string]interface{}))["id"].(string)
+	t.loggerhook.Reset()
+
+	path := "/node/" + id
+	dl := "?download&del"
+	t.checkFile(t.url+path+dl, path, &deleter, 9, id, []byte("foobarbaz"))
+	t.checkNodeDeleted(id, t.noRole)
+}
+
 func (t *TestSuite) TestStoreMIMEMultipartFilenameFormat() {
 	partsuffix := ` filename="myfile.txt"`
 	format := "gasbomb"
@@ -1065,7 +1082,7 @@ func (t *TestSuite) TestGetNodeFailPerms() {
 	)
 }
 
-func (t *TestSuite) TestGetNodeFailSeekAndLength() {
+func (t *TestSuite) TestGetFileFailSeekAndLength() {
 	body := t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
 		"OAuth "+t.kBaseAdmin.token, 374, 200)
 	id := (body["data"].(map[string]interface{}))["id"].(string)
@@ -1104,6 +1121,49 @@ func (t *TestSuite) TestGetNodeFailSeekAndLength() {
 	)
 }
 
+func (t *TestSuite) TestGetFileFailDelete() {
+	// only tests differences between the standard path and the delete path
+	body := t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
+		"OAuth "+t.noRole.token, 374, 200)
+	id := (body["data"].(map[string]interface{}))["id"].(string)
+	t.loggerhook.Reset()
+
+	uid := uuid.New()
+	body2 := t.get(t.url+"/node/"+uid.String()+"?download&del", &t.noRole, 75, 404)
+	t.checkError(body2, 404, "Node not found")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/"+uid.String(), 404, &t.noRole.user,
+		"Node not found", mtmap(), false},
+	)
+	body3 := t.get(t.url+"/node/"+id+"?download&del", &t.noRole2, 78, 401)
+	t.checkError(body3, 401, "User Unauthorized")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, &t.noRole2.user,
+		"User Unauthorized", mtmap(), false},
+	)
+	body4 := t.get(t.url+"/node/"+id+"?download&del", nil, 78, 401)
+	t.checkError(body4, 401, "User Unauthorized")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, nil,
+		"User Unauthorized", mtmap(), false},
+	)
+	// add user 2 to read acl
+	t.req("PUT", t.url+"/node/"+id+"/acl/read?users="+t.noRole2.user, nil,
+		"Oauth "+t.noRole.token, 441, 200)
+	t.loggerhook.Reset()
+	body5 := t.get(t.url+"/node/"+id+"?download&del", &t.noRole2, 94, 401)
+	t.checkError(body5, 401, "Only node owners can delete nodes")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, &t.noRole2.user,
+		"Only node owners can delete nodes", mtmap(), false},
+	)
+	// make node public
+	t.req("PUT", t.url+"/node/"+id+"/acl/public_read", nil,
+		"Oauth "+t.noRole.token, 440, 200)
+	t.loggerhook.Reset()
+	body6 := t.get(t.url+"/node/"+id+"?download&del", nil, 94, 401)
+	t.checkError(body6, 401, "Only node owners can delete nodes")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, nil,
+		"Only node owners can delete nodes", mtmap(), false},
+	)
+}
+
 func (t *TestSuite) TestUnexpectedError() {
 	defer t.createTestBucket()
 	body := t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
@@ -1150,6 +1210,7 @@ func (t *TestSuite) TestDeleteNode() {
 	t.checkLogs(logEvent{logrus.InfoLevel, "DELETE", "/node/" + id, 200, &t.noRole.user,
 		"request complete", mtmap(), true},
 	)
+	t.checkNodeDeleted(id, t.noRole)
 
 	// test delete as admin and with trailing slash
 	body = t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
@@ -1164,6 +1225,15 @@ func (t *TestSuite) TestDeleteNode() {
 	t.checkLogs(logEvent{logrus.InfoLevel, "DELETE", "/node/" + id + "/", 200, &t.kBaseAdmin.user,
 		"request complete", mtmap(), true},
 	)
+	t.checkNodeDeleted(id, t.noRole)
+}
+
+func (t *TestSuite) checkNodeDeleted(id string, user User) {
+	body := t.get(t.url+"/node/"+id, &user, 75, 404)
+	t.checkError(body, 404, "Node not found")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 404, &user.user,
+		"Node not found", mtmap(), false},
+	)
 }
 
 func (t *TestSuite) TestDeleteNodeFail() {
diff --git a/service/server.go b/service/server.go
index e331aef..b8f33ef 100644
--- a/service/server.go
+++ b/service/server.go
@@ -514,6 +514,11 @@ func (s *Server) getNode(w http.ResponseWriter, r *http.Request) {
 	user := getUser(r)
 	download := download(r.URL)
 	if download != "" {
+		del, err := checkDel(s, user, id, r.URL)
+		if err != nil {
+			writeError(le, err, w)
+			return
+		}
 		seek, length, err := getSeekAndLengthFromQuery(r.URL)
 		if err != nil {
 			writeError(le, err, w)
@@ -534,6 +539,9 @@ func (s *Server) getNode(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("content-length", strconv.FormatInt(size, 10))
 		w.Header().Set("content-type", "application/octet-stream")
 		io.Copy(w, datareader)
+		if del {
+			s.store.DeleteNode(*user, *id) // ignore errors since file is written to output
+		}
 	} else {
 		node, err := s.store.Get(user, *id)
 		if err != nil {
@@ -544,6 +552,29 @@ func (s *Server) getNode(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+func checkDel(s *Server, user *auth.User, id *uuid.UUID, u *url.URL) (del bool, err error) {
+	delete := false
+	if _, ok := u.Query()["del"]; ok {
+		delete = true
+		// If we want to move the deletion into the core logic it could go like this:
+		// * pass in deletion flag and a pre-write function that accepts a node
+		// * do the ACL checks, etc.
+		// * the pre-write function is called, which writes the headers to the writer
+		//   * the core logic should not know the writer is a http.ResponseWriter
+		// * stream the file
+		// * delete the node
+		// not worth the trouble for now
+		node, err := s.store.Get(user, *id)
+		if err != nil {
+			return false, err
+		}
+		if user == nil || (user.GetUserName() != node.Owner.AccountName && !user.IsAdmin()) {
+			return false, NewUnauthorizedCustomError("Only node owners can delete nodes")
+		}
+	}
+	return delete, nil
+}
+
 func getSeekAndLengthFromQuery(u *url.URL) (uint64, uint64, error) {
 	seek, err := getUint64FromQuery(u, "seek")
 	if err != nil {

From 60864d4aa7f28dfc02f06d7738a5dac04644310a Mon Sep 17 00:00:00 2001
From: Gavin <gaprice@lbl.gov>
Date: Thu, 16 May 2024 08:59:27 -0700
Subject: [PATCH 4/5] Get auth token from cookie

```
$ curl --cookie "kbase_session=$KBASE_CI_TOKEN" http://localhost:8080/node/06cc2b59-b33c-460a-8332-151a509b6d20/acl
{
  "data": {
    "delete": [
      "057abd01-95db-4500-9541-8ce2a925c3e3"
    ],
    "owner": "057abd01-95db-4500-9541-8ce2a925c3e3",
    "public": {
      "delete": false,
      "read": false,
      "write": false
    },
    "read": [
      "057abd01-95db-4500-9541-8ce2a925c3e3"
    ],
    "write": [
      "057abd01-95db-4500-9541-8ce2a925c3e3"
    ]
  },
  "error": null,
  "status": 200
}
$ curl --cookie "kbase_sessionx=$KBASE_CI_TOKEN" http://localhost:8080/node/06cc2b59-b33c-460a-8332-151a509b6d20/acl
{
  "data": null,
  "error": [
    "User Unauthorized"
  ],
  "status": 401
}
```
---
 README.md                            |  13 ++-
 RELEASE_NOTES.md                     |   1 +
 app/blobstore.go                     |   2 +-
 config/config.go                     |   8 ++
 config/config_test.go                |   5 +
 deploy.cfg.example                   |   3 +
 deployment/conf/deployment.cfg.templ |   3 +
 docker-compose.yml                   |  43 ++-------
 service/errortypes.go                |  20 +++-
 service/integration_test.go          | 134 +++++++++++++++++++++++----
 service/server.go                    |  16 ++++
 11 files changed, 189 insertions(+), 59 deletions(-)

diff --git a/README.md b/README.md
index cebf585..bb13968 100644
--- a/README.md
+++ b/README.md
@@ -101,8 +101,17 @@ This data structure is identical to Shock's error data structure.
 
 # API
 
-Requests are authenticated by including the header `Authorization: OAuth <kbase token>` in the
-request.
+## Authentication
+
+Requests are authenticated by including the header `Authorization: OAuth <kbase token>` or
+including a cookie with the value of `<kbase token>` in the request.
+
+The names of cookies that the server will check are set in the deployment configuration file.
+
+The header takes precedence, then each cookie in the list in the configuration file in order.
+
+Note that for backwards compatibility, incorrect or invalid authentication headers respond with a
+400 HTTP code. Invalid cookies respond with the appropriate 401 code.
 
 ## Root
 
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index fb7efea..15ee12a 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,6 +1,7 @@
 # 0.1.4
 
 * Added the `del` param when downloading the file from a node.
+* The Blobstore will now look for auth tokens in cookies specified in the deployment configuration.
 
 # 0.1.3
 
diff --git a/app/blobstore.go b/app/blobstore.go
index f20e075..7cb9870 100644
--- a/app/blobstore.go
+++ b/app/blobstore.go
@@ -19,7 +19,7 @@ import (
 
 const (
 	name        = "blobstore"
-	version     = "0.1.3"
+	version     = "0.1.4"
 	shockname   = "Shock"
 	shockver    = "0.9.6" // do not increment
 	deprecation = "The id and version fields are deprecated."
diff --git a/config/config.go b/config/config.go
index 7a51ac4..05dd7df 100644
--- a/config/config.go
+++ b/config/config.go
@@ -46,6 +46,9 @@ const (
 	// KeyAuthAdminRoles is the configuration key where the value is comma-delimited auth server
 	// roles that denote that a user is a blobstore admin
 	KeyAuthAdminRoles = "kbase-auth-admin-roles"
+	// KeyAuthTokenCookies is the configuartion key where the value is comma-delimited cookie
+	// names where the serivce should look for authentication tokens.
+	KeyAuthTokenCookies = "kbase-auth-token-cookies"
 	// KeyDontTrustXIPHeaders is the configuration key where the value determines whether to
 	// distrust the X-Forwarded-For and X-Real-IP headers (true) or not (anything else).
 	KeyDontTrustXIPHeaders = "dont-trust-x-ip-headers"
@@ -82,6 +85,9 @@ type Config struct {
 	// AuthAdminRoles are the auth server roles that denote that a user is a blobstore admin.
 	// It is never nil but may be empty.
 	AuthAdminRoles *[]string
+	// AuthTokenCookies are the cookie names to check for auth tokens.
+	// It is never nil but may be empty.
+	AuthTokenCookies *[]string
 	// DontTrustXIPHeaders determines whether to distrust the X-Forwarded-For and X-Real-IP
 	// headers.
 	DontTrustXIPHeaders bool
@@ -112,6 +118,7 @@ func New(configFilePath string) (*Config, error) {
 	s3region, err := getString(err, configFilePath, sec, KeyS3Region, true)
 	authurl, err := getURL(err, configFilePath, sec, KeyAuthURL)
 	roles, err := getStringList(err, configFilePath, sec, KeyAuthAdminRoles)
+	cookies, err := getStringList(err, configFilePath, sec, KeyAuthTokenCookies)
 	xip, err := getString(err, configFilePath, sec, KeyDontTrustXIPHeaders, false)
 	if err != nil {
 		return nil, err
@@ -137,6 +144,7 @@ func New(configFilePath string) (*Config, error) {
 			S3Region:            s3region,
 			AuthURL:             authurl,
 			AuthAdminRoles:      roles,
+			AuthTokenCookies:    cookies,
 			DontTrustXIPHeaders: "true" == xip,
 		},
 		nil
diff --git a/config/config_test.go b/config/config_test.go
index 032143d..0351037 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -93,6 +93,7 @@ func (t *TestSuite) TestMinimalConfig() {
 		S3DisableSSLVerify:  false,
 		AuthURL:             u,
 		AuthAdminRoles:      &[]string{},
+		AuthTokenCookies:    &[]string{},
 		DontTrustXIPHeaders: false,
 	}
 	t.Equal(&expected, cfg, "incorrect config")
@@ -114,6 +115,7 @@ func (t *TestSuite) TestMinimalConfigWhitespaceFields() {
 		"s3-region =       us-west-1    \t   ",
 		"kbase-auth-url = https://kbase.us/authyauth",
 		"kbase-auth-admin-roles =    \t     ",
+		"kbase-auth-token-cookies =    \t     ",
 		"dont-trust-x-ip-headers =      \t     ",
 	)
 	cfg, err := New(filePath)
@@ -132,6 +134,7 @@ func (t *TestSuite) TestMinimalConfigWhitespaceFields() {
 		S3DisableSSLVerify:  false,
 		AuthURL:             u,
 		AuthAdminRoles:      &[]string{},
+		AuthTokenCookies:    &[]string{},
 		DontTrustXIPHeaders: false,
 	}
 	t.Equal(&expected, cfg, "incorrect config")
@@ -153,6 +156,7 @@ func (t *TestSuite) TestMaximalConfig() {
 		"s3-disable-ssl-verify=     true    ",
 		"kbase-auth-url = https://kbase.us/authyauth",
 		"kbase-auth-admin-roles =    \t     ,    foo   , \tbar\t , ,  baz ,,",
+		"kbase-auth-token-cookies =    \t   , chokkiechip   , \toreoinmilk\t , , earwaxNsnot ,,",
 		"dont-trust-x-ip-headers =     true   \t  ",
 	)
 	cfg, err := New(filePath)
@@ -173,6 +177,7 @@ func (t *TestSuite) TestMaximalConfig() {
 		S3Region:            "us-west-1",
 		AuthURL:             u,
 		AuthAdminRoles:      &[]string{"foo", "bar", "baz"},
+		AuthTokenCookies:    &[]string{"chokkiechip", "oreoinmilk", "earwaxNsnot"},
 		DontTrustXIPHeaders: true,
 	}
 	t.Equal(&expected, cfg, "incorrect config")
diff --git a/deploy.cfg.example b/deploy.cfg.example
index 04af651..256acf6 100644
--- a/deploy.cfg.example
+++ b/deploy.cfg.example
@@ -30,6 +30,9 @@ s3-region = us-west-1
 kbase-auth-url = https://kbase.us/services/auth
 # KBase auth server custom roles that denote the user is a blobstore admin. Comma delimited.
 kbase-auth-admin-roles = KBASE_ADMIN, BLOBSTORE_ADMIN
+# A list of comma separated cookie names to check for authentication tokens.
+# The authentication header is checked first, then each cookie in the list in order.
+# kbase-auth-token-cookies = 
 
 # If "true", make the server ignore the X-Forwarded-For and X-Real-IP headers. Otherwise
 # (the default behavior), the logged IP address for a request, in order of precedence, is
diff --git a/deployment/conf/deployment.cfg.templ b/deployment/conf/deployment.cfg.templ
index bfccc16..5f0b3e2 100644
--- a/deployment/conf/deployment.cfg.templ
+++ b/deployment/conf/deployment.cfg.templ
@@ -28,6 +28,9 @@ s3-disable-ssl-verify = {{ default .Env.s3_disable_ssl_verify "false" }}
 kbase-auth-url = {{ default .Env.kbase_auth_url "https://ci.kbase.us/services/auth" }}
 # KBase auth server custom roles that denote the user is a blobstore admin. Comma delimited.
 kbase-auth-admin-roles = {{ default .Env.kbase_auth_admin_roles "KBASE_ADMIN, BLOBSTORE_ADMIN" }}
+# A list of comma separated cookie names to check for authentication tokens.
+# The authentication header is checked first, then each cookie in the list in order.
+kbase-auth-token-cookies = {{ default .Env.kbase_auth_token_cookies "kbase_session, kbase_session_backup" }}
 
 # If "true", make the server ignore the X-Forwarded-For and X-Real-IP headers. Otherwise
 # (the default behavior), the logged IP address for a request, in order of precedence, is
diff --git a/docker-compose.yml b/docker-compose.yml
index 1487361..dd1605c 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -5,13 +5,13 @@ version: "3.1"
 # that is started up and polled
 services:
   kbase_blobstore:
-    image: kbase/blobstore:latest
+    build: .
     ports:
       - "8080:8080"
     environment:
       blobstore_host: 0.0.0.0:8080
-      mongo_host: localhost:27017
-      mongo_database: dc_blobstore_test
+      mongodb_host: mongo:27017
+      mongodb_database: dc_blobstore_test
       kbase_auth_url: https://ci.kbase.us/services/auth
       kbase_auth_admin_roles: KBASE_ADMIN,BLOBSTORE_ADMIN
       s3_host: minio:9000
@@ -24,9 +24,7 @@ services:
     command:
       - "-multiline"
       - "-wait"
-      - "tcp://ci-mongo:27017"
-      - "-wait"
-      - "tcp://mongoinit:8080"
+      - "tcp://mongo:27017"
       - "-wait"
       - "tcp://minio:9000"
       - "-timeout"
@@ -36,37 +34,10 @@ services:
       - "/kb/deployment/blobstore/blobstore"
       - "--conf"
       - "/kb/deployment/conf/deployment.cfg"
-      # If you needed to pass in context for template evaluation you would put something like
-      # these lines that tell dockerize to hit github for an INI style file for the context
-    # - "-env"
-    # - "https://raw.githubusercontent.com/kbase/mini_kb/master/deployment/conf/tauth2-minikb.yml"
-      # If the -env URL needs authentication you would use an -env-header directive that specified
-      # either the hard coded string for the header, or a path to a file that contains the header
-      # string ( used for working with docker secrets files)
-    # - "-env-header"
-    # - "AUTHORIZATION:authtokenvalue"
-      # or for a path to a secrets file:
-    # - "env-header"
-    # - "/run/secrets/authheader"
-      # If your server is using self-signed certs, or otherwise problematic for cert validation
-      # you can add the following flag:
-    # - "-validateCert=false"
-    depends_on: ["ci-mongo", "mongoinit", "minio"]
-
-  mongoinit:
-    image: kbase/db_initialize:latest
-    entrypoint:
-      - "/kb/deployment/bin/dockerize.sh"
-      - "-wait"
-      - "tcp://ci-mongo:27017"
-      - "-timeout"
-      - "120s"
-    depends_on: [ "ci-mongo" ]
+    depends_on: ["mongo", "minio"]
   
-  ci-mongo:
-    image: mongo:2
-    command:
-      - "--smallfiles"
+  mongo:
+    image: mongo:3.6
     ports:
       - "27017:27017"
 
diff --git a/service/errortypes.go b/service/errortypes.go
index 8c7ebe4..2ef6951 100644
--- a/service/errortypes.go
+++ b/service/errortypes.go
@@ -18,7 +18,7 @@ const (
 // special explanation in the error string.
 type UnauthorizedCustomError string
 
-// UnauthorizedCustomError creates a new UnauthorizedCustomError.
+// NewUnauthorizedCustomError creates a new UnauthorizedCustomError.
 func NewUnauthorizedCustomError(err string) *UnauthorizedCustomError {
 	e := UnauthorizedCustomError(err)
 	return &e
@@ -28,6 +28,20 @@ func (e *UnauthorizedCustomError) Error() string {
 	return string(*e)
 }
 
+// InvalidTokenCustomError denotes that an invalid token was submitted and special explanation
+// is needed in the error string.
+type InvalidTokenCustomError string
+
+// NewInvalidTokenCustomError creates a new InvalidTokenCustomError.
+func NewInvalidTokenCustomError(err string) *InvalidTokenCustomError {
+	e := InvalidTokenCustomError(err)
+	return &e
+}
+
+func (e *InvalidTokenCustomError) Error() string {
+	return string(*e)
+}
+
 func translateError(err error) (code int, errstr string) {
 	// not sure about this approach. Alternative is to add some state to every error that
 	// can be mapped to a code, and I'm not super thrilled about that either.
@@ -35,6 +49,8 @@ func translateError(err error) (code int, errstr string) {
 	case *auth.InvalidTokenError:
 		// Shock compatibility, should be 401
 		return http.StatusBadRequest, invalidAuthHeader
+	case *InvalidTokenCustomError:
+		return http.StatusUnauthorized, t.Error()
 	case *core.NoBlobError:
 		return http.StatusNotFound, "Node not found"
 	case *core.UnauthorizedError:
@@ -45,7 +61,7 @@ func translateError(err error) (code int, errstr string) {
 		return http.StatusBadRequest, "Users that are not node owners can only delete " +
 			"themselves from ACLs."
 	case *UnauthorizedCustomError:
-		return http.StatusUnauthorized, t.Error()
+		return http.StatusForbidden, t.Error()
 	case *auth.InvalidUserError:
 		// no equivalent shock error, it accepts any string as a username
 		return http.StatusBadRequest, t.Error()
diff --git a/service/integration_test.go b/service/integration_test.go
index 4877f94..3b9cdca 100644
--- a/service/integration_test.go
+++ b/service/integration_test.go
@@ -80,20 +80,20 @@ func (t *TestSuite) SetupSuite() {
 	logrus.SetOutput(ioutil.Discard)
 	t.loggerhook = logrust.NewGlobal()
 
-	roles := []string{adminRole, blobstoreRole}
 	serv, err := New(
 		&config.Config{
-			Host:           "foo", // not used
-			MongoHost:      "localhost:" + strconv.Itoa(t.mongo.GetPort()),
-			MongoDatabase:  testDB,
-			S3Host:         "localhost:" + strconv.Itoa(t.minio.GetPort()),
-			S3Bucket:       testBucket,
-			S3AccessKey:    "ackey",
-			S3AccessSecret: "sooporsecret",
-			S3Region:       "us-west-1",
-			S3DisableSSL:   true,
-			AuthURL:        &authurl,
-			AuthAdminRoles: &roles,
+			Host:             "foo", // not used
+			MongoHost:        "localhost:" + strconv.Itoa(t.mongo.GetPort()),
+			MongoDatabase:    testDB,
+			S3Host:           "localhost:" + strconv.Itoa(t.minio.GetPort()),
+			S3Bucket:         testBucket,
+			S3AccessKey:      "ackey",
+			S3AccessSecret:   "sooporsecret",
+			S3Region:         "us-west-1",
+			S3DisableSSL:     true,
+			AuthURL:          &authurl,
+			AuthAdminRoles:   &[]string{adminRole, blobstoreRole},
+			AuthTokenCookies: &[]string{"cookie1", "cookie2", "cookie3"},
 		},
 		ServerStaticConf{
 			ServerName:          "servn",
@@ -443,6 +443,104 @@ func (t *TestSuite) checkXIPLogs(xFF, xRIP, ip string) {
 	t.loggerhook.Reset()
 }
 
+// Since the middleware applies to every method, testing it exhaustively is unreasonable.
+// Here we test it using a node get for simpliciaty.
+// These tests are expected to exercise the middleware, not the specific data method.
+
+func (t *TestSuite) TestAuthenticationMiddleWare() {
+	fake := "fake"
+	mt := ""
+	ws := "      "
+	t.testAuthenticationMiddleWare(
+		t.noRole.user, &t.noRole.token, &fake, &fake, &fake,
+		t.noRole2.user, nil, &t.noRole2.token, &fake, &fake,
+	)
+	t.testAuthenticationMiddleWare(
+		t.noRole.user, nil, &t.noRole.token, &fake, &fake,
+		t.noRole2.user, &t.noRole2.token, &fake, &fake, &fake,
+	)
+	t.testAuthenticationMiddleWare(
+		t.noRole.user, &mt, nil, &t.noRole.token, &fake,
+		t.noRole2.user, nil, nil, nil, &t.noRole2.token,
+	)
+	t.testAuthenticationMiddleWare(
+		t.noRole.user, &mt, &ws, nil, &t.noRole.token,
+		t.noRole2.user, &mt, &ws, &t.noRole2.token, &fake,
+	)
+	t.testAuthenticationMiddleWare(
+		t.noRole.user, &mt, &mt, &ws, &t.noRole.token,
+		t.noRole2.user, &mt, &mt, &t.noRole2.token, &fake,
+	)
+}
+
+func (t *TestSuite) testAuthenticationMiddleWare(
+	pstUser string, pstHeader *string, pstCookie1 *string, pstCookie2 *string, pstCookie3 *string,
+	getUser string, getHeader *string, getCookie1 *string, getCookie2 *string, getCookie3 *string,
+) {
+	req1, err := http.NewRequest("POST", t.url+"/node", strings.NewReader("d"))
+	t.Nil(err, "unexpected error")
+	addAuthHeader(req1, pstHeader)
+	addCookie(req1, "cookie1", pstCookie1)
+	addCookie(req1, "cookie2", pstCookie2)
+	addCookie(req1, "cookie3", pstCookie3)
+	body := t.requestToJSON(req1, 374, 200)
+
+	t.checkLogs(logEvent{logrus.InfoLevel, "POST", "/node", 200, &pstUser,
+		"request complete", mtmap(), false},
+	)
+	data := body["data"].(map[string]interface{})
+	id := data["id"].(string)
+	t.Equal(body["status"], float64(200), "incorrect status")
+
+	// add read for a different user
+	body = t.req("PUT", t.url+"/node/"+id+"/acl/read?users="+t.noRole2.user, nil,
+		"Oauth "+t.noRole.token, 441, 200)
+	t.loggerhook.Reset()
+
+	// now get the node with a different user
+	req2, err := http.NewRequest("GET", t.url+"/node/" + id, nil)
+	t.Nil(err, "unexpected error")
+	addAuthHeader(req2, getHeader)
+	addCookie(req2, "cookie1", getCookie1)
+	addCookie(req2, "cookie2", getCookie2)
+	addCookie(req2, "cookie3", getCookie3)
+	body = t.requestToJSON(req2, 374, 200)
+
+	t.checkLogs(logEvent{logrus.InfoLevel, "GET", "/node/" + id, 200, &getUser,
+		"request complete", mtmap(), false},
+	)
+	t.Equal(body["status"], float64(200), "incorrect status")
+}
+
+func addCookie(req *http.Request, name string, val *string) {
+	if val != nil {
+		req.AddCookie(&http.Cookie{Name: name, Value: *val})
+	}
+}
+
+func addAuthHeader(req *http.Request, token *string) {
+	if token != nil {
+		if *token == "" {
+			req.Header.Set("authorization", "")
+		} else {
+			req.Header.Set("authorization", "Oauth "+*token)
+		}
+	}
+}
+
+func (t *TestSuite) TestAuthenticationMiddleWareFailBadCookie() {
+	req1, err := http.NewRequest("POST", t.url+"/node", strings.NewReader("d"))
+	t.Nil(err, "unexpected error")
+	c := "fake"
+	addCookie(req1, "cookie2", &c)
+	addCookie(req1, "cookie3", &c)
+	body := t.requestToJSON(req1, 125, 401)
+	t.checkError(body, 401, "KBase auth server reported token was invalid from cookie cookie2")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "POST", "/node", 401, nil,
+		"KBase auth server reported token was invalid from cookie cookie2", mtmap(), false},
+	)
+}
+
 func (t *TestSuite) TestStoreAndGetWithFilename() {
 	// check whitespace
 	body := t.req("POST", t.url+"/node?filename=%20%20myfile%20%20",
@@ -1148,18 +1246,18 @@ func (t *TestSuite) TestGetFileFailDelete() {
 	t.req("PUT", t.url+"/node/"+id+"/acl/read?users="+t.noRole2.user, nil,
 		"Oauth "+t.noRole.token, 441, 200)
 	t.loggerhook.Reset()
-	body5 := t.get(t.url+"/node/"+id+"?download&del", &t.noRole2, 94, 401)
-	t.checkError(body5, 401, "Only node owners can delete nodes")
-	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, &t.noRole2.user,
+	body5 := t.get(t.url+"/node/"+id+"?download&del", &t.noRole2, 94, 403)
+	t.checkError(body5, 403, "Only node owners can delete nodes")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 403, &t.noRole2.user,
 		"Only node owners can delete nodes", mtmap(), false},
 	)
 	// make node public
 	t.req("PUT", t.url+"/node/"+id+"/acl/public_read", nil,
 		"Oauth "+t.noRole.token, 440, 200)
 	t.loggerhook.Reset()
-	body6 := t.get(t.url+"/node/"+id+"?download&del", nil, 94, 401)
-	t.checkError(body6, 401, "Only node owners can delete nodes")
-	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, nil,
+	body6 := t.get(t.url+"/node/"+id+"?download&del", nil, 94, 403)
+	t.checkError(body6, 403, "Only node owners can delete nodes")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 403, nil,
 		"Only node owners can delete nodes", mtmap(), false},
 	)
 }
diff --git a/service/server.go b/service/server.go
index b8f33ef..6909337 100644
--- a/service/server.go
+++ b/service/server.go
@@ -68,6 +68,7 @@ type Server struct {
 	auth             *authcache.Cache
 	store            *core.BlobStore
 	ignoreXIPheaders bool
+	authCookieNames  *[]string
 }
 
 // New create a new server.
@@ -88,6 +89,7 @@ func New(cfg *config.Config, sconf ServerStaticConf) (*Server, error) {
 		auth:             deps.AuthCache,
 		store:            deps.BlobStore,
 		ignoreXIPheaders: cfg.DontTrustXIPHeaders,
+		authCookieNames:  cfg.AuthTokenCookies,
 	}
 	router.NotFoundHandler = http.HandlerFunc(s.notFoundHandler)
 	router.MethodNotAllowedHandler = http.HandlerFunc(s.notAllowedHandler)
@@ -218,11 +220,25 @@ func (s *Server) authLogMiddleWare(next http.Handler) http.Handler {
 			writeErrorWithCode(le, err.Error(), 400, w)
 			return
 		}
+		cookieName := ""
+		if token == "" {
+			for _, cn := range *s.authCookieNames {
+				cookie, err := r.Cookie(cn)
+				if err == nil && strings.TrimSpace(cookie.Value) != "" {
+					token = cookie.Value
+					cookieName = cn
+					break
+				}
+			}
+		}
 		var user *auth.User
 		if token != "" {
 			var err error
 			user, err = s.auth.GetUser(le, token)
 			if err != nil {
+				if cookieName != "" {
+					err = NewInvalidTokenCustomError(err.Error()+" from cookie "+cookieName)
+				}
 				writeError(le, err, w)
 				return
 			}

From b9da32eef18ed0c4df3524807f5e63dbe38429eb Mon Sep 17 00:00:00 2001
From: Gavin <gaprice@lbl.gov>
Date: Thu, 16 May 2024 16:22:11 -0700
Subject: [PATCH 5/5] Restrict filename and format strings

... to a reasonable character set. This avoids characters that may cause issues in mac, linux, and windows
---
 README.md                   | 10 ++--
 RELEASE_NOTES.md            |  3 ++
 core/values/values.go       | 42 ++++++++++++++--
 core/values/values_test.go  | 29 +++++++++--
 service/integration_test.go | 99 +++++++++++++++++++++++++++++--------
 5 files changed, 151 insertions(+), 32 deletions(-)

diff --git a/README.md b/README.md
index bb13968..a0bf3fc 100644
--- a/README.md
+++ b/README.md
@@ -150,8 +150,10 @@ curl -H "Authorization: OAuth $KBASE_TOKEN" -T mylittlefile
   "http://<host>/node?filename=mylittlefile&format=text"
 ```
 
-`filename` can be at most 256 characters with no control characters.  
-`format` can be at most 100 characters with no control characters.
+`filename` can be at most 256 characters consisting of only unicode alphanumerics, space, and
+the characters `[ ] ( ) = . - _`.  
+`format` can be at most 100 characters consisting of only unicode alphanumerics and
+the characters `- _`. 
 
 ## Copy a node
 ```
@@ -265,8 +267,8 @@ The form **may** contain a part called `format` where the part contents are the
 file, equivalent to the `format` query parameter for the standard upload method and with the same
 restrictions. The `format` part **MUST** come before the `upload` part.
 
-Any file name provided in the `Content-Disposition` header can be at most 256 characters with no
-control characters.
+Any file name provided in the `Content-Disposition` header has the same restrictions as the
+filename parameter for the standard upload method.
 
 ### Curl example
 
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 15ee12a..38306c3 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -2,6 +2,9 @@
 
 * Added the `del` param when downloading the file from a node.
 * The Blobstore will now look for auth tokens in cookies specified in the deployment configuration.
+* The filename and format strings are now much more restrictive in regard to allowed contents.
+  See the API documenation for details.
+  * Any extant data is not affected and will be returned when requested as normal.
 
 # 0.1.3
 
diff --git a/core/values/values.go b/core/values/values.go
index 6735e28..b7f243e 100644
--- a/core/values/values.go
+++ b/core/values/values.go
@@ -9,11 +9,23 @@ import (
 )
 
 const (
-	maxFileNameSize = 256
-	maxFormatSize   = 100
+	maxFileNameSize    = 256
+	maxFormatSize      = 100
+	allowedFileChars   = "[]()=-_. "
+	allowedFormatChars = "_-"
 )
 
 var md5regex = regexp.MustCompile("^[a-fA-F0-9]{32}$")
+var fileCharsSet = createAllowedCharsSet(allowedFileChars)
+var formatCharsSet = createAllowedCharsSet(allowedFormatChars)
+
+func createAllowedCharsSet(allowedSpecialChars string) map[rune]struct{} {
+	allowedCharsSet := make(map[rune]struct{})
+	for _, char := range allowedSpecialChars {
+		allowedCharsSet[char] = struct{}{}
+	}
+	return allowedCharsSet
+}
 
 // MD5 contains a valid MD5 string.
 type MD5 struct {
@@ -53,7 +65,7 @@ type FileName struct {
 
 // NewFileName creates a new filename.
 func NewFileName(name string) (*FileName, error) {
-	name, err := checkString(name, "File name", maxFileNameSize)
+	name, err := checkString(name, "File name", maxFileNameSize, fileCharsSet)
 	if err != nil {
 		return nil, err
 	}
@@ -72,7 +84,7 @@ type FileFormat struct {
 
 // NewFileFormat creates a new filename.
 func NewFileFormat(name string) (*FileFormat, error) {
-	name, err := checkString(name, "File format", maxFormatSize)
+	name, err := checkString(name, "File format", maxFormatSize, formatCharsSet)
 	if err != nil {
 		return nil, err
 	}
@@ -84,17 +96,37 @@ func (fn *FileFormat) GetFileFormat() string {
 	return fn.fileFormat
 }
 
-func checkString(s string, name string, maxSize int) (string, error) {
+func checkString(
+	s string, name string, maxSize int, allowedSpcChars map[rune]struct{},
+) (string, error) {
 	s = strings.TrimSpace(s)
 	if len(s) > maxSize {
 		return "", NewIllegalInputError(fmt.Sprintf("%s is > %d bytes", name, maxSize))
 	}
+	// check for control characters first to avoid returning / logging a string with control chars
 	if containsControlChar(s) {
 		return "", NewIllegalInputError(fmt.Sprintf("%s contains control characters", name))
 	}
+	for _, r := range(s) {
+		if !isAllowedChar(r, allowedSpcChars) {
+			return "", NewIllegalInputError(
+				fmt.Sprintf("%s string %s contains an illegal character: %q", name, s, r),
+			)
+		}
+	}
 	return s, nil
 }
 
+func isAllowedChar(r rune, allowedSpcChars map[rune]struct{}) bool {
+	if unicode.IsLetter(r) || unicode.IsDigit(r) {
+		return true
+	}
+	if _, exists := allowedSpcChars[r]; exists {
+		return true
+	}
+	return false
+}
+
 func containsControlChar(s string) bool {
 	for _, c := range s {
 		if unicode.IsControl(c) {
diff --git a/core/values/values_test.go b/core/values/values_test.go
index 8105a1e..20dc6e4 100644
--- a/core/values/values_test.go
+++ b/core/values/values_test.go
@@ -51,7 +51,7 @@ func TestFileName(t *testing.T) {
 }
 
 func fileNameString() string {
-	s := "ab9 8&#']K[1*(&)ꢇ]" // 20 bytes
+	s := "ab9_8४.[]( )1=-ꢇ" // 20 bytes
 
 	b := strings.Builder{}
 	for i := 0; i < 12; i++ {
@@ -72,18 +72,34 @@ func TestFileNameFail(t *testing.T) {
 	assert.Nil(t, fn, "expected error")
 	assert.Equal(t, NewIllegalInputError("File name contains control characters"), err,
 		"incorrect error")
+	for _, ch := range []string{"*", ":", "?", "|", "✈", "🐱", "∞", "€", "◊"} {
+		fn, err = NewFileName("abc"+ch+"neg")
+		assert.Nil(t, fn, "expected error")
+		errstr := "File name string abc"+ch+"neg contains an illegal character: '"+ch+"'"
+		assert.Equal(t, NewIllegalInputError(errstr), err, "incorrect error")
+	}
 }
 
 func TestFileFormat(t *testing.T) {
-	fns := fileNameString()[:100]
+	fns := formatString()
 	assert.Equal(t, 100, len(fns), "incorrect length")
 	fn, err := NewFileFormat("    \t       " + fns + "    \t       ")
 	assert.Nil(t, err, "unexpected error")
 	assert.Equal(t, fns, fn.GetFileFormat())
 }
 
+func formatString() string {
+	s := "ab9_8४K3Z23o1n-ꢇ" // 20 bytes
+
+	b := strings.Builder{}
+	for i := 0; i < 5; i++ {
+		b.Write([]byte(s))
+	}
+	return b.String()
+}
+
 func TestFileFormatFail(t *testing.T) {
-	fns := fileNameString()[:100]
+	fns := formatString()
 	assert.Equal(t, 100, len(fns), "incorrect length")
 	fn, err := NewFileFormat(fns + "a")
 	assert.Nil(t, fn, "expected error")
@@ -93,4 +109,11 @@ func TestFileFormatFail(t *testing.T) {
 	assert.Nil(t, fn, "expected error")
 	assert.Equal(t, NewIllegalInputError("File format contains control characters"), err,
 		"incorrect error")
+	badchrs := []string{"*", ":", "?", "|", "✈", "🐱", "∞", "€", "◊", "=", "[", "]", "(", ")", "."}
+	for _, ch := range badchrs {
+		fn, err = NewFileFormat("abc"+ch+"neg")
+		assert.Nil(t, fn, "expected error")
+		errstr := "File format string abc"+ch+"neg contains an illegal character: '"+ch+"'"
+		assert.Equal(t, NewIllegalInputError(errstr), err, "incorrect error")
+	}
 }
diff --git a/service/integration_test.go b/service/integration_test.go
index 3b9cdca..616a890 100644
--- a/service/integration_test.go
+++ b/service/integration_test.go
@@ -72,9 +72,7 @@ func (t *TestSuite) SetupSuite() {
 
 	t.mongo, t.mongoclient = t.setupMongo(tcfg)
 	t.minio = t.setupMinio(tcfg)
-	auth, authurl := t.setupAuth(tcfg)
-	t.auth = auth
-	t.authurl = authurl
+	t.auth, t.authurl = t.setupAuth(tcfg)
 	t.setUpUsersAndRoles()
 
 	logrus.SetOutput(ioutil.Discard)
@@ -91,7 +89,7 @@ func (t *TestSuite) SetupSuite() {
 			S3AccessSecret:   "sooporsecret",
 			S3Region:         "us-west-1",
 			S3DisableSSL:     true,
-			AuthURL:          &authurl,
+			AuthURL:          &t.authurl,
 			AuthAdminRoles:   &[]string{adminRole, blobstoreRole},
 			AuthTokenCookies: &[]string{"cookie1", "cookie2", "cookie3"},
 		},
@@ -543,8 +541,8 @@ func (t *TestSuite) TestAuthenticationMiddleWareFailBadCookie() {
 
 func (t *TestSuite) TestStoreAndGetWithFilename() {
 	// check whitespace
-	body := t.req("POST", t.url+"/node?filename=%20%20myfile%20%20",
-		strings.NewReader("foobarbaz"), "     OAuth    "+t.noRole.token+"      ", 380, 200)
+	body := t.req("POST", t.url+"/node?filename=%20%20my_fileթ%20%20",
+		strings.NewReader("foobarbaz"), "     OAuth    "+t.noRole.token+"      ", 383, 200)
 
 	t.checkLogs(logEvent{logrus.InfoLevel, "POST", "/node", 200, &t.noRole.user,
 		"request complete", mtmap(), false},
@@ -565,7 +563,7 @@ func (t *TestSuite) TestStoreAndGetWithFilename() {
 			"format":     "",
 			"file": map[string]interface{}{
 				"checksum": map[string]interface{}{"md5": "6df23dc03f9b54cc38a0fc1483df6e21"},
-				"name":     "myfile",
+				"name":     "my_fileթ",
 				"size":     float64(9),
 			},
 		},
@@ -583,26 +581,26 @@ func (t *TestSuite) TestStoreAndGetWithFilename() {
 			"format":        "",
 			"file": map[string]interface{}{
 				"checksum": map[string]interface{}{"md5": "6df23dc03f9b54cc38a0fc1483df6e21"},
-				"name":     "myfile",
+				"name":     "my_fileթ",
 				"size":     float64(9),
 			},
 		},
 		"error":  nil,
 		"status": float64(200),
 	}
-	t.checkNode(id, &t.noRole, 380, expected2)
+	t.checkNode(id, &t.noRole, 383, expected2)
 
 	path1 := "/node/" + id
 	path2 := path1 + "/"
-	t.checkFile(t.url+path1+"?download", path1, &t.noRole, 9, "myfile", []byte("foobarbaz"))
-	t.checkFile(t.url+path2+"?download", path2, &t.noRole, 9, "myfile", []byte("foobarbaz"))
+	t.checkFile(t.url+path1+"?download", path1, &t.noRole, 9, "my_fileթ", []byte("foobarbaz"))
+	t.checkFile(t.url+path2+"?download", path2, &t.noRole, 9, "my_fileթ", []byte("foobarbaz"))
 	t.checkFile(t.url+path1+"?download_raw", path1, &t.noRole, 9, "", []byte("foobarbaz"))
 	t.checkFile(t.url+path2+"?download_raw", path2, &t.noRole, 9, "", []byte("foobarbaz"))
 }
 
 func (t *TestSuite) TestStoreAndGetNodeAsAdminWithFormatAndTrailingSlashAndSeekAndLength() {
-	body := t.req("POST", t.url+"/node/?format=JSON", strings.NewReader("foobarbaz"),
-		"oauth "+t.noRole.token, 378, 200)
+	body := t.req("POST", t.url+"/node/?format=J-SONթ", strings.NewReader("foobarbaz"),
+		"oauth "+t.noRole.token, 381, 200)
 	t.checkLogs(logEvent{logrus.InfoLevel, "POST", "/node/", 200, ptr("noroles"),
 		"request complete", mtmap(), false},
 	)
@@ -617,7 +615,7 @@ func (t *TestSuite) TestStoreAndGetNodeAsAdminWithFormatAndTrailingSlashAndSeekA
 			"created_on":    time,
 			"last_modified": time,
 			"id":            id,
-			"format":        "JSON",
+			"format":        "J-SONթ",
 			"file": map[string]interface{}{
 				"checksum": map[string]interface{}{"md5": "6df23dc03f9b54cc38a0fc1483df6e21"},
 				"name":     "",
@@ -628,7 +626,7 @@ func (t *TestSuite) TestStoreAndGetNodeAsAdminWithFormatAndTrailingSlashAndSeekA
 		"status": float64(200),
 	}
 	path := "/node/" + id
-	t.checkNode(id, &t.stdRole, 378, expected)
+	t.checkNode(id, &t.stdRole, 381, expected)
 	dl := fmt.Sprintf("?download&seek=2&length=5")
 	dlnolen := fmt.Sprintf("?download&seek=6")
 	dlr := fmt.Sprintf("?download_raw&seek=7&length=100")
@@ -637,7 +635,7 @@ func (t *TestSuite) TestStoreAndGetNodeAsAdminWithFormatAndTrailingSlashAndSeekA
 	t.checkFile(t.url+path+dl, path, &t.stdRole, 5, id, []byte("obarb"))
 	t.checkFile(t.url+path+dlnolen, path, &t.stdRole, 3, id, []byte("baz"))
 	t.checkFile(t.url+path+dlr, path, &t.stdRole, 2, "", []byte("az"))
-	t.checkNode(id, &t.kBaseAdmin, 378, expected)
+	t.checkNode(id, &t.kBaseAdmin, 381, expected)
 	t.checkFile(t.url+path+noopdl, path, &t.kBaseAdmin, 9, id, []byte("foobarbaz"))
 	t.checkFile(t.url+path+dlrlen, path, &t.kBaseAdmin, 7, "", []byte("foobarb"))
 }
@@ -660,9 +658,9 @@ func (t *TestSuite) testGetFileWithDelete(deleter User) {
 }
 
 func (t *TestSuite) TestStoreMIMEMultipartFilenameFormat() {
-	partsuffix := ` filename="myfile.txt"`
-	format := "gasbomb"
-	t.storeMIMEMultipart(partsuffix, &format, "myfile.txt", 392)
+	partsuffix := ` filename="my-file.txt"`
+	format := "gas_bomb"
+	t.storeMIMEMultipart(partsuffix, &format, "my-file.txt", 394)
 }
 
 func (t *TestSuite) TestStoreMIMEMultipartWhitespaceFileNameFormat() {
@@ -768,6 +766,53 @@ func (t *TestSuite) storeMIMEMultipart(
 	t.checkFile(t.url+path2+"?download_raw", path2, &t.noRole, 11, "", []byte("foobarbazba"))
 }
 
+
+func (t *TestSuite) TestGetNodeAndFileWithIllegalFileNameAndFormat() {
+	// In versions of the Blobstore before 0.1.4, filename and format strings were unconstrained
+	// other than they couldn't contain control characters.
+	// That data may still exist in older systems, so we need to test that the data can be
+	// returned, regardless of whether it meets the now stricter criteria for incoming data.
+	body := t.req("POST", t.url+"/node/", strings.NewReader("f"),
+		"oauth "+t.noRole.token, 374, 200)
+	t.checkLogs(logEvent{logrus.InfoLevel, "POST", "/node/", 200, &t.noRole.user,
+		"request complete", mtmap(), false},
+	)
+
+	data := body["data"].(map[string]interface{})
+	id := data["id"].(string)
+	
+	for _, ch := range []string{"*", "|", ":", "¥"} {
+		updatedoc := map[string]interface{}{
+			"$set": map[string]string{
+				"fname": "abc"+ch+"def",
+				"fmt": "uvw"+ch+"xyz",
+			},
+		}
+		t.mongoclient.Database(testDB).Collection("nodes").UpdateOne(
+			nil,
+			map[string]string{"id": id},
+			updatedoc,
+		)
+		req, err := http.NewRequest(http.MethodGet, t.url+"/node/"+id, nil)
+		t.Nil(err, "unexpected error")
+		req.Header.Set("authorization", "oauth "+t.noRole.token)
+		resp, err := http.DefaultClient.Do(req)
+		t.Nil(err, "unexpected error")
+		b, err := ioutil.ReadAll(resp.Body)
+		t.Nil(err, "unexpected error")
+		var body map[string]interface{}
+		json.Unmarshal(b, &body)
+		data = body["data"].(map[string]interface{})
+		t.Equal("uvw"+ch+"xyz", data["format"], "incorrect format")
+		file := data["file"].(map[string]interface{})
+		t.Equal("abc"+ch+"def", file["name"], "incorrect filename")
+		t.loggerhook.Reset()
+		t.checkFile(
+			t.url+"/node/"+id+"?download", "/node/"+id, &t.noRole, 1, "abc"+ch+"def", []byte("f"),
+		)
+	}
+}
+
 func (t *TestSuite) TestStoreMIMEMultipartFailContentLength() {
 	// don't load MIME this way, sticks everything in memory
 	for _, cl := range []string{"", "not a number", "-1"} {
@@ -858,7 +903,7 @@ func (t *TestSuite) TestFormNodeFailEarlyEOFAfterFormat() {
 		"--supahboundary\n"+
 			`Content-Disposition: form-data; name="format"`+"\n"+
 			"\n"+
-			"format here\n",
+			"formathere\n",
 	))
 	t.Nil(err, "unexpected error")
 	req.Header.Set("Content-Type", "multipart/form-data; boundary=supahboundary")
@@ -1112,6 +1157,13 @@ func (t *TestSuite) TestStoreBadFileName() {
 	t.checkLogs(logEvent{logrus.ErrorLevel, "POST", "/node", 400, &t.noRole.user,
 		"File name contains control characters", mtmap(), false},
 	)
+
+	body = t.req("POST", t.url+"/node?filename=foo☃bar", strings.NewReader("foobarbaz"),
+		"oauth "+t.noRole.token, 124, 400)
+	t.checkError(body, 400, "File name string foo☃bar contains an illegal character: '☃'")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "POST", "/node", 400, &t.noRole.user,
+		"File name string foo☃bar contains an illegal character: '☃'", mtmap(), false},
+)
 }
 
 func (t *TestSuite) TestStoreBadFileFormat() {
@@ -1121,6 +1173,13 @@ func (t *TestSuite) TestStoreBadFileFormat() {
 	t.checkLogs(logEvent{logrus.ErrorLevel, "POST", "/node", 400, &t.noRole.user,
 		"File format contains control characters", mtmap(), false},
 	)
+
+	body = t.req("POST", t.url+"/node?format=foo∑bar", strings.NewReader("foobarbaz"),
+		"oauth "+t.noRole.token, 126, 400)
+	t.checkError(body, 400, "File format string foo∑bar contains an illegal character: '∑'")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "POST", "/node", 400, &t.noRole.user,
+		"File format string foo∑bar contains an illegal character: '∑'", mtmap(), false},
+)
 }
 
 func (t *TestSuite) TestGetNodeBadID() {